Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis pull request makes extensive improvements to the rate limit logs feature. Function signatures were updated to accept additional log types, and numerous new components, hooks, schemas, and utility functions were added. The changes enhance charts, controls, filtering, detailed log views, TRPC router methods, and internal data querying. Legacy components were removed, and UI elements such as navbar spacing, table borders, and icons were refined. In addition, new unit tests have been introduced to improve the reliability of filter and logs query functionalities. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant LC as LogsClient
participant Q as useRatelimitLogsQuery
participant TR as TRPC Router
participant CH as ClickHouse
U->>LC: Opens logs page
LC->>Q: Request historical & real-time logs with filters
Q->>TR: Forward query parameters
TR->>CH: Query ClickHouse for logs data
CH-->>TR: Return logs data
TR-->>Q: Return processed logs
Q-->>LC: Supply logs data
LC->>U: Render logs table and details
Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
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 (
|
|
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: 3
🧹 Nitpick comments (3)
internal/clickhouse/src/ratelimits.ts (2)
137-145: LGTM! Consider using Object.fromEntries for parameter reduction.The spread operator usage is safe here, but consider using
Object.fromEntriesfor a more explicit transformation:- ...(args.identifiers?.reduce( - (acc, i, index) => ({ - // biome-ignore lint/performance/noAccumulatingSpread: it's okay here - ...acc, - [`identifierValue_${index}`]: i.value, - }), - {}, - ) ?? {}), + ...(args.identifiers + ? Object.fromEntries( + args.identifiers.map((i, index) => [ + `identifierValue_${index}`, + i.value, + ]) + ) + : {}),
313-314: Consider adding an index for the JOIN condition.The LEFT JOIN between
raw_ratelimits_v1andraw_api_requests_v1onrequest_idmight benefit from an index.ALTER TABLE metrics.raw_api_requests_v1 ADD INDEX idx_request_id request_id TYPE minmax GRANULARITY 1;apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/hooks/use-filters.ts (1)
12-35: Consider supporting partial hours or fractional durations.
YourparseAsRelativeTimefunction enforces the format(\d+[hdm])+, which excludes partial hours (e.g.,2.5h). If this is intentional, it’s fine; otherwise, consider allowing fractional or more flexible time notations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/control-cloud/index.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/hooks/use-filters.ts(1 hunks)internal/clickhouse/src/ratelimits.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Test Packages / Test ./internal/clickhouse
- 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)
internal/clickhouse/src/ratelimits.ts (1)
221-239: Review sensitive data exposure in response schema.The schema includes potentially sensitive information such as request headers, request body, and response body. Consider:
- Implementing field-level access control
- Masking sensitive data in headers and body
- Adding logging for sensitive data access
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/hooks/use-filters.ts (1)
37-64: Confirm whether 'startsWith' or 'endsWith' operators have been intentionally removed.
Previously, there were references to'startsWith'and'endsWith'operators. This parser only allows"is"and"contains". Please ensure these two are no longer needed in your filter schema or reintroduce them here if necessary.apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/control-cloud/index.tsx (2)
32-37: Verify that numeric timestamps are in milliseconds.
Theformatfunction from date-fns expects a Unix timestamp in milliseconds if you’re passing a numeric value. Double-check that thevalueis indeed in ms; otherwise, the displayed date/time will be incorrect.
72-110: Implementation looks solid!
YourControlPillcomponent is clearly structured, handles focus states well, and prevents default backspace/delete navigation.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/control-cloud/index.tsx
Show resolved
Hide resolved
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/control-cloud/index.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-refresh.tsx (1)
24-45: Add error handling for query invalidation.While the refresh logic is well-implemented, consider adding error handling for the query invalidation calls to gracefully handle potential failures.
const handleSwitch = () => { if (isLoading) { return; } const isLiveBefore = Boolean(isLive); setIsLoading(true); toggleLive(false); - ratelimit.logs.query.invalidate(); - ratelimit.logs.queryRatelimitTimeseries.invalidate(); + Promise.all([ + ratelimit.logs.query.invalidate(), + ratelimit.logs.queryRatelimitTimeseries.invalidate(), + ]).catch((error) => { + console.error('Failed to invalidate queries:', error); + // Optionally show a user-friendly error message + }); if (refreshTimeout) { clearTimeout(refreshTimeout); } const timeout = setTimeout(() => { setIsLoading(false); if (isLiveBefore) { toggleLive(true); } }, REFRESH_TIMEOUT_MS); setRefreshTimeout(timeout); };apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/log-details/components/log-footer.tsx (1)
53-53: Consider using optional chaining for consistency.For consistency with other null checks in the codebase, consider using optional chaining.
- content: getRequestHeader(log, "user-agent") ?? "", + content: getRequestHeader(log, "user-agent")?.toString() ?? "",apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-filters/components/identifiers-filter.tsx (1)
21-30: Memoize the select callback for better performance.The select callback in the TRPC query options could be memoized to prevent unnecessary re-renders.
+const selectIdentifiers = useCallback((identifiers: string[] | undefined) => { + return identifiers + ? identifiers.map((identifier, index) => ({ + id: index + 1, + path: identifier, + checked: false, + })) + : []; +}, []); { select(identifiers) { - return identifiers - ? identifiers.map((identifier, index) => ({ - id: index + 1, - path: identifier, - checked: false, - })) - : []; + return selectIdentifiers(identifiers); }, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-filters/components/identifiers-filter.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-refresh.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/log-details/components/log-footer.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/log-details/components/log-meta.tsx(1 hunks)apps/dashboard/lib/trpc/routers/logs/query-logs/utils.ts(1 hunks)apps/dashboard/lib/trpc/routers/logs/query-timeseries/utils.ts(1 hunks)apps/dashboard/lib/trpc/routers/logs/utils/getTimestampFromRelative.ts(0 hunks)apps/dashboard/lib/utils.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/dashboard/lib/trpc/routers/logs/utils/getTimestampFromRelative.ts
✅ Files skipped from review due to trivial changes (2)
- apps/dashboard/lib/trpc/routers/logs/query-logs/utils.ts
- apps/dashboard/lib/trpc/routers/logs/query-timeseries/utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/lib/utils.ts
🧰 Additional context used
📓 Learnings (1)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/log-details/components/log-footer.tsx (1)
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2866
File: apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/log-details/components/log-footer.tsx:85-98
Timestamp: 2025-02-05T12:56:44.873Z
Learning: The RequestResponseDetails component in the ratelimit logs UI already handles empty content cases by preventing rendering when content is empty, so no additional empty state handling is needed in the parent components.
🪛 Biome (1.9.4)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-filters/components/identifiers-filter.tsx
[error] 124-137: A form label must be associated with an input.
Consider adding a for or htmlFor attribute to the label element or moving the input element to inside the label element.
(lint/a11y/noLabelWithoutControl)
[error] 144-156: A form label must be associated with an input.
Consider adding a for or htmlFor attribute to the label element or moving the input element to inside the label element.
(lint/a11y/noLabelWithoutControl)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/cache
- 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 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 (10)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-refresh.tsx (4)
1-11: LGTM! Clean imports and well-defined constants.The imports are well-organized, and the timeout duration is properly extracted into a constant.
12-18: LGTM! Proper state management with race condition prevention.The component correctly implements state management using hooks and includes timeout tracking to prevent race conditions.
19-22: Consider using a modified key combination for the refresh shortcut.The current implementation using a plain 'r' key could conflict with:
- Browser's refresh shortcut (Ctrl+R/Cmd+R)
- Text input fields where users might type 'r'
Consider using a modifier key combination (e.g., Alt+R or Shift+R) to avoid these conflicts.
47-64: LGTM! Well-implemented UI with good user feedback.The button implementation includes:
- Proper loading state visualization
- Clear disabled state handling
- Good user feedback through animation
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/log-details/components/log-meta.tsx (3)
14-15: Remove console.error and improve error handling.Production code should not contain console.error statements. Consider implementing proper error tracking.
- console.error("Failed to copy to clipboard:", error); toast.error("Failed to copy to clipboard");
24-24: LGTM! Good empty content handling.The fallback to "" when content is null provides good user feedback.
25-32: LGTM! Good accessibility implementation.The copy button implementation is well done with:
- Proper aria-label for screen readers
- Hover state for better UX
- Clear visual feedback with the Copy icon
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/log-details/components/log-footer.tsx (2)
61-63: LGTM! Good null handling for outcome.The default outcome handling is well implemented with a clear default value.
85-98: Skip commenting on empty permissions handling.Based on the retrieved learning, the RequestResponseDetails component already handles empty content cases internally, so no additional empty state handling is needed here.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-filters/components/identifiers-filter.tsx (1)
124-137: Fix accessibility issues with form labels.The labels need proper association with their checkboxes for better accessibility.
<label + htmlFor="select-all" className="flex items-center gap-4 px-4 pb-2 pt-4 cursor-pointer" - aria-checked={checkboxes.every((checkbox) => checkbox.checked)} onKeyDown={handleKeyDown} > <Checkbox + id="select-all" checked={checkboxes.every((checkbox) => checkbox.checked)} className="size-4 rounded border-gray-4 [&_svg]:size-3" onClick={handleSelectAll} /> {checkboxes.map((checkbox, index) => ( <label key={checkbox.id} + htmlFor={`checkbox-${checkbox.id}`} className="flex gap-[18px] items-center py-1 cursor-pointer" - aria-checked={checkbox.checked} onKeyDown={handleKeyDown} > <Checkbox + id={`checkbox-${checkbox.id}`} checked={checkbox.checked} className="size-4 rounded border-gray-4 [&_svg]:size-3" onClick={() => handleCheckboxChange(index)} />Also applies to: 144-156
🧰 Tools
🪛 Biome (1.9.4)
[error] 124-137: A form label must be associated with an input.
Consider adding a
fororhtmlForattribute to the label element or moving the input element to inside the label element.(lint/a11y/noLabelWithoutControl)
...espaceId]/logs/components/controls/components/logs-filters/components/identifiers-filter.tsx
Show resolved
Hide resolved
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 to display error messages for log retrieval failures.LogsChartLoadingcomponent to show loading animations while fetching log data.useFetchRatelimitTimeseries, for fetching and formatting timeseries data related to rate limits.RatelimitLogsChartcomponent to visualize rate limit logs in a bar chart format.DatetimePopoverfor selecting time ranges with predefined options.LogsSearchcomponent for searching logs using AI, enhancing filtering capabilities.FilterCheckboxcomponent for customizable checkbox filtering.FiltersPopoverfor managing filter options in a user-friendly interface.LogsLiveSwitchcomponent for toggling between live and historical log views.LogsRefreshcomponent for refreshing logs with a button interface.LogsFilterscomponent to manage and display active filters.Bug Fixes
UI Enhancements