Conversation
![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
🧹 Nitpick comments (3)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/components/logs-actions/index.tsx (1)
47-96: Consider adding keyboard shortcuts to menu items.The menu items are well-structured with clear actions and visual feedback. Consider enhancing accessibility by adding keyboard shortcuts to the menu items.
const items: MenuItem[] = [ { id: "logs", label: "Go to logs", + shortcut: "G L", icon: <Layers3 size="md-regular" />, onClick(e) {apps/dashboard/app/(app)/ratelimits/[namespaceId]/_components/table-action-popover.tsx (2)
42-102: Consider adding Home/End key support for keyboard navigation.The keyboard navigation is well-implemented but could be enhanced with support for Home/End keys to jump to the first/last enabled items.
switch (e.key) { + case "Home": { + e.preventDefault(); + const firstEnabledIndex = items.findIndex(item => !item.disabled); + if (firstEnabledIndex >= 0) { + setFocusIndex(firstEnabledIndex); + menuItems.current[firstEnabledIndex]?.focus(); + } + break; + } + case "End": { + e.preventDefault(); + const lastEnabledIndex = items.findLastIndex(item => !item.disabled); + if (lastEnabledIndex >= 0) { + setFocusIndex(lastEnabledIndex); + menuItems.current[lastEnabledIndex]?.focus(); + } + break; + } case "Tab": {
139-140: Remove unnecessary biome-ignore comment.The click event handler is properly complemented with keyboard event handling, making the lint ignore unnecessary.
- // biome-ignore lint/a11y/useKeyWithClickEvents: <explanation> <div
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_components/delete-dialog.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/_components/table-action-popover.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/components/logs-actions/index.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- 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/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 Packages / Test ./internal/encryption
- GitHub Check: Test Packages / Test ./internal/billing
- 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 (4)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/components/logs-actions/index.tsx (1)
30-45: LGTM! Clean implementation of URL parameter handling.The
getTimeParamsfunction effectively constructs URL parameters for time-based filtering, with proper null checks and parameter appending.apps/dashboard/app/(app)/ratelimits/[namespaceId]/_components/delete-dialog.tsx (2)
19-24: Fix the form validation logic.The current validation using
v === vwill always return true, making the validation ineffective.const formSchema = z.object({ identifier: z .string() - // biome-ignore lint/suspicious/noSelfCompare: <explanation> - .refine((v) => v === v, "Please confirm the identifier"), + .refine((v) => v.trim() === identifier.trim(), "Please confirm the identifier"), });
69-75: Improve error handling in form submission.The error is only logged to console without proper cleanup or user feedback.
const onSubmit = async () => { try { await deleteOverride.mutateAsync({ id: overrideId }); } catch (error) { - console.error("Delete error:", error); + // Error is already handled by the mutation's onError callback + onOpenChange(false); } };apps/dashboard/app/(app)/ratelimits/[namespaceId]/_components/table-action-popover.tsx (1)
6-13: LGTM! Well-defined type for menu items.The
MenuItemtype is comprehensive and includes all necessary properties for menu items.
.../(app)/ratelimits/[namespaceId]/_overview/components/table/components/logs-actions/index.tsx
Show resolved
Hide resolved
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_components/delete-dialog.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
apps/dashboard/lib/trpc/routers/ratelimit/createOverride.ts (1)
107-109:⚠️ Potential issueFix potential ID mismatch in return value.
When updating an existing override, the function always returns the new ID instead of the existing override's ID. This could lead to inconsistencies in the calling code.
Apply this fix:
return { - id, + id: existing ? existing.id : id, };
🧹 Nitpick comments (1)
apps/dashboard/lib/trpc/routers/ratelimit/createOverride.ts (1)
47-74: Consider optimizing the override creation/update logic.While the current implementation is functional, there are several opportunities for improvement:
- Consider using an upsert operation instead of separate update/insert operations.
- Use the table's where builder consistently instead of raw SQL conditions.
- Include createdAt in the update operation to maintain consistency.
Here's a suggested optimization:
- const existing = await tx.query.ratelimitOverrides.findFirst({ - where: (table, { and, eq }) => - and(eq(table.namespaceId, namespace.id), eq(table.identifier, input.identifier)), - }); - - if (existing) { - await tx - .update(schema.ratelimitOverrides) - .set({ - limit: input.limit, - duration: input.duration, - async: input.async ?? false, - updatedAt: new Date(), - deletedAt: null, - }) - .where(sql`namespace_id = ${namespace.id} AND identifier = ${input.identifier}`); - } else { - await tx.insert(schema.ratelimitOverrides).values({ - id, - workspaceId: ctx.workspace.id, - namespaceId: namespace.id, - identifier: input.identifier, - limit: input.limit, - duration: input.duration, - async: input.async ?? false, - createdAt: new Date(), - }); - } + const result = await tx + .insert(schema.ratelimitOverrides) + .values({ + id, + workspaceId: ctx.workspace.id, + namespaceId: namespace.id, + identifier: input.identifier, + limit: input.limit, + duration: input.duration, + async: input.async ?? false, + createdAt: new Date(), + }) + .onConflictDoUpdate({ + target: [schema.ratelimitOverrides.namespaceId, schema.ratelimitOverrides.identifier], + set: { + limit: input.limit, + duration: input.duration, + async: input.async ?? false, + updatedAt: new Date(), + deletedAt: null, + }, + }) + .returning({ id: schema.ratelimitOverrides.id }); + + const overrideId = result[0]?.id ?? id;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_components/table-action-popover.tsx(1 hunks)apps/dashboard/lib/trpc/routers/ratelimit/createOverride.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/_components/table-action-popover.tsx
⏰ Context from checks skipped due to timeout of 90000ms (18)
- 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 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 Packages / Test ./internal/encryption
- GitHub Check: Test Packages / Test ./internal/billing
- 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: Analyze (javascript-typescript)
- GitHub Check: Build / Build
- GitHub Check: autofix
🔇 Additional comments (3)
apps/dashboard/lib/trpc/routers/ratelimit/createOverride.ts (3)
5-5: LGTM! Import consolidation improves code organization.The combined import of db-related utilities enhances code maintainability.
20-42: LGTM! Robust namespace validation with proper error handling.The validation logic ensures workspace ownership, namespace existence, and handles soft-deletes appropriately. Error messages are user-friendly and include support contact information.
75-97: LGTM! Comprehensive audit logging with proper event differentiation.The audit logging implementation effectively:
- Distinguishes between create and update operations
- Provides clear event descriptions
- Maintains proper resource tracking
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/charts/bar-chart/index.tsx (1)
230-239: Simplify ReferenceArea coordinate calculations.The current implementation has complex conditional logic and unsafe type coercion. It can be simplified while maintaining type safety.
Consider this cleaner implementation:
- x1={ - selection.start && selection.end - ? Math.min(Number(selection.start), Number(selection.end)) - : selection.start - } - x2={ - selection.start && selection.end - ? Math.max(Number(selection.start), Number(selection.end)) - : selection.end - } + x1={Math.min(+selection.start, +selection.end)} + x2={Math.max(+selection.start, +selection.end)}This is safer because:
- The unary
+operator is more idiomatic for number conversion- We already know both values exist due to the condition on line 227
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/charts/bar-chart/index.tsx(4 hunks)apps/dashboard/package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Go API Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/charts/bar-chart/index.tsx (1)
16-16: LGTM! Good choice of library for timezone handling.The addition of
date-fns-tzis appropriate for handling timezone offsets consistently.apps/dashboard/package.json (1)
77-77:Details
❓ Verification inconclusive
Verify the date-fns-tz version and check for security advisories.
Let's ensure we're using a secure and up-to-date version of the package.
Also, verify compatibility with the main date-fns package:
🏁 Script executed:
#!/bin/bash # Check npm for latest version and security advisories npm view date-fns-tz versions --json | jq -r '.[-1]' npm audit date-fns-tz@3.2.0Length of output: 439
🌐 Web query:
What is the recommended version of date-fns-tz to use with date-fns v3.6.0?💡 Result:
The recommended version of
date-fns-tzto use withdate-fns v3.6.0is version 3.x, specifically v3.2.0 or later[9]. This compatibility is confirmed by:
Explicit version alignment:
date-fns-tzwas designed fordate-fnsv3.x[3][8]- Both libraries follow semantic versioning, ensuring compatibility within major versions[7]
Known conflicts:
Newer versions ofdate-fns(v4+) introduced breaking changes like native time zone support via@date-fns/tz, making olderdate-fns-tzversions incompatible[1][2]. However, this doesn't affect v3.x users.For projects using
date-fns@3.6.0, install with:npm install date-fns@3.6.0 date-fns-tz@3.2.0Key considerations:
- Avoid mixing
date-fnsv4+ withdate-fns-tzdue to architectural changes in time zone handling[1][2][9]- If upgrading to
date-fnsv4+, migrate to@date-fns/tzinstead ofdate-fns-tz[1][2]Citations:
- 1: https://github.com/date-fns/date-fns/releases
- 2: https://www.npmjs.com/package/@date-fns/tz
- 3: https://www.npmjs.com/package/date-fns-tz
- 4: https://security.snyk.io/package/npm/date-fns-tz
- 5: https://www.npmjs.com/package/date-fns/v/3.0.6
- 6: https://discourse.nodered.org/t/using-date-fns-tz-additional-module/90435
- 7: https://date-fns.org
- 8: marnusw/date-fns-tz#260
- 9: https://pipedream.com/community/t/why-am-i-suddenly-experiencing-errors-with-the-date-fns-package-in-my-code/9958
- 10: https://date-fns.org/docs/Getting-Started
Dependency Version Verified – Please Ensure a Lockfile Is Present for Security Audit
- The
date-fns-tzversion"3.2.0"is confirmed as the latest release and is the recommended version for compatibility withdate-fnsv3.6.0.- Note that the npm audit check failed due to a missing lockfile. To perform a full security audit, please create a lockfile (e.g., run
npm install --package-lock-only) and then re-run the audit.
...dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/charts/bar-chart/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/dashboard/components/logs/chart/utils/format-timestamp.ts (2)
36-38: Add JSDoc to improve code documentation.The function logic is correct, but adding JSDoc would improve code documentation.
+/** + * Converts a Unix timestamp in microseconds to a Date object. + * @param unix - Unix timestamp in microseconds (as string or number) + * @returns Date object + */ const unixMicroToDate = (unix: string | number): Date => { return fromUnixTime(Number(unix) / 1000 / 1000); };
46-49: Add JSDoc to improve code documentation.The function logic is correct and the addition of seconds in the format string improves precision.
+/** + * Formats a timestamp for tooltip display. + * Handles both Unix microsecond timestamps and regular timestamps. + * @param value - Timestamp value (Unix microseconds or regular timestamp) + * @returns Formatted timestamp string in "MMM dd HH:mm:ss" format + */ export const formatTimestampTooltip = (value: string | number) => { const date = isUnixMicro(value) ? unixMicroToDate(value) : new Date(value); return format(date, "MMM dd HH:mm:ss"); };internal/clickhouse/src/logs.ts (1)
259-280: Use a more precise approach for monthly intervals if needed.
DefiningMONTHinmsPerUnitas2592000_000(30 days) may be acceptable for rough intervals but can cause drift if you need actual month-length accuracy. Consider a more dynamic calculation if precise month-based reporting is required.internal/clickhouse/src/ratelimits.ts (1)
106-115: Clarify approximate month interval usage.
Using a fixed value of 30 days for monthly intervals may introduce subtle inaccuracies. If precise monthly boundaries are required, consider a more robust method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/charts/bar-chart/index.tsx(3 hunks)apps/dashboard/components/logs/chart/utils/format-timestamp.ts(2 hunks)internal/clickhouse/src/logs.ts(3 hunks)internal/clickhouse/src/ratelimits.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/charts/bar-chart/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (13)
- 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 Packages / Test ./internal/hash
- GitHub Check: Test Go API Local / test_agent_local
- 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
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
apps/dashboard/components/logs/chart/utils/format-timestamp.ts (1)
2-2: LGTM!The addition of
fromUnixTimefromdate-fnsis appropriate for the new Unix timestamp conversion functionality.internal/clickhouse/src/logs.ts (3)
192-192: Confirm holistic usage of integer timestamps.
You've replaceddateTimeToUnixwithz.number().int(), which aligns with an integer-based approach for timestamps. Please check if all existing references and downstream transformations still function correctly with numeric timestamps rather than custom date conversions.
283-283: Verify ClickHouse version support fortoUnixTimestamp64Milli.
Ensure your ClickHouse environment supports this function; older versions might not.
295-297: Great improvement in time-bounded queries.
UsingFROM … TO … STEP …with millisecond resolution improves accuracy for large date ranges. No further issues noted here.internal/clickhouse/src/ratelimits.ts (3)
35-35: Shift to integer timestamps is consistent.
Switching fromdateTimeToUnixtoz.number().int()is in line with the new approach in other modules. Ensure places expecting a date/time conversion are updated accordingly.
118-118: Check function availability fortoUnixTimestamp64Milli.
Confirm that your deployment’s ClickHouse version includes support for this function.
128-130: Clean use of FROM–TO–STEP logic.
Your approach to define the time range with exact millisecond steps is neat and consistent with similar logic in logs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/dashboard/components/logs/chart/components/logs-chart-error.tsx (2)
3-25: Consider enhancing the error handling capabilities.The component could be more flexible and informative with these improvements:
- Accept a custom error message prop instead of hardcoding "Could not retrieve logs"
- Add an error icon for better visual feedback
- Make the height responsive instead of fixed at 50px
Here's a suggested implementation:
-export const LogsChartError = () => { +interface LogsChartErrorProps { + message?: string; + height?: number; +} + +export const LogsChartError = ({ + message = "Could not retrieve logs", + height = 50 +}: LogsChartErrorProps) => { return ( <div className="w-full relative"> <div className="px-2 text-accent-11 font-mono absolute top-0 text-xxs w-full flex justify-between opacity-50"> {Array(5) .fill(0) .map((_, i) => ( // biome-ignore lint/suspicious/noArrayIndexKey: it's okay <div key={i} className="z-10"> --:-- </div> ))} </div> - <ResponsiveContainer height={50} className="border-b border-gray-4" width="100%"> + <ResponsiveContainer height={height} className="border-b border-gray-4" width="100%"> <div className="flex-1 flex items-center justify-center h-full"> <div className="flex flex-col items-center gap-2"> - <span className="text-sm text-accent-9">Could not retrieve logs</span> + <div className="flex items-center gap-2"> + <svg + className="w-4 h-4 text-accent-9" + fill="none" + stroke="currentColor" + viewBox="0 0 24 24" + > + <path + strokeLinecap="round" + strokeLinejoin="round" + strokeWidth={2} + d="M12 8v4m0 4h.01M21 12a9 9 0 11-18 0 9 9 0 0118 0z" + /> + </svg> + <span className="text-sm text-accent-9">{message}</span> + </div> </div> </div> </ResponsiveContainer> </div> ); };
6-15: Consider improving the decorative header.The current implementation uses "--:--" placeholders which might be confusing. Consider using more meaningful placeholders or removing them if they don't serve a clear purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/components/logs/chart/components/logs-chart-error.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Test Packages / Test ./packages/rbac
- 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 Packages / Test ./internal/encryption
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Test Go API Local / test_agent_local
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
apps/dashboard/components/logs/chart/components/logs-chart-error.tsx (1)
17-20: LGTM! The styling improvements enhance readability.The updated layout with flex containers and improved typography provides better visual hierarchy and alignment.
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
Style
Refactor