Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis pull request introduces significant enhancements to the logs-v2 feature, focusing on improving user interaction, filter management, and search capabilities. The changes span multiple components and modules, introducing new functionality like AI-powered log searching, advanced keyboard navigation for filters, and a more modular approach to filter configuration. Key improvements include restructuring filter components, adding keyboard accessibility, implementing an OpenAI-powered natural language search, and centralizing filter type definitions. Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 0
🧹 Nitpick comments (3)
apps/dashboard/lib/trpc/routers/logs/llm-search.ts (3)
64-69: Enhance rate limit error handlingThe rate limit error handling could be improved by including a retry-after header in the response.
if ((error as any).response?.status === 429) { + const retryAfter = (error as any).response?.headers?.['retry-after'] || '60'; throw new TRPCError({ code: "TOO_MANY_REQUESTS", - message: "Search rate limit exceeded. Please try again in a few minutes.", + message: `Search rate limit exceeded. Please try again in ${retryAfter} seconds.`, }); }
79-103: Consider extracting workspace verification to middlewareThe workspace verification logic could be moved to a reusable middleware to maintain DRY principles and ensure consistent workspace access checks across routes.
Example middleware implementation:
const withWorkspace = middleware(async ({ ctx, next }) => { const workspace = await db.query.workspaces .findFirst({ where: (table, { and, eq, isNull }) => and(eq(table.tenantId, ctx.tenant.id), isNull(table.deletedAt)), }) .catch((_err) => { throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: "Failed to verify workspace access. Please try again or contact support@unkey.dev if this persists.", }); }); if (!workspace) { throw new TRPCError({ code: "NOT_FOUND", message: "Workspace not found, please contact support using support@unkey.dev.", }); } return next({ ctx: { ...ctx, workspace, }, }); });
106-142: Simplify filter transformation logicThe switch case in
transformFiltersToQuerySearchParamscould be simplified and made more type-safe.function transformFiltersToQuerySearchParams( result: z.infer<typeof filterOutputSchema>, ): QuerySearchParams { - const output: QuerySearchParams = { + const initialOutput: QuerySearchParams = { host: null, requestId: null, methods: null, paths: null, status: null, }; - for (const filter of result.filters) { - const filterValues = filter.filters.map((f) => ({ + return result.filters.reduce((output, filter) => { + const filterValues = filter.filters.map((f) => ({ operator: f.operator, value: f.value, })); - switch (filter.field) { - case "host": - case "requestId": - if (filter.filters.length > 0) { - output[filter.field] = filterValues[0]; - } - break; + if (filter.filters.length === 0) return output; - case "methods": - case "paths": - case "status": - if (filter.filters.length > 0) { - output[filter.field] = filterValues; - } - break; - } - } + const isSingleValueField = filter.field === 'host' || filter.field === 'requestId'; + output[filter.field] = isSingleValueField ? filterValues[0] : filterValues; - return output; + return output; + }, initialOutput); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/lib/trpc/routers/logs/llm-search.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- 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 API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
apps/dashboard/lib/trpc/routers/logs/llm-search.ts (4)
1-16: LGTM! Well-structured imports and OpenAI client initialization.The conditional initialization of the OpenAI client based on API key availability is a good practice for development environments.
25-25: Incorrect OpenAI model nameThe model name
"gpt-4o-2024-08-06"appears to be incorrect. Please verify and update it to the correct model name.
55-59: Avoid logging sensitive user inputs in error messagesLogging user inputs in error messages can lead to potential exposure of sensitive information.
1-213: Overall implementation is well-structuredThe implementation of AI-powered natural language query parsing is well-designed with proper error handling, rate limiting, and clear system prompts. After addressing the critical issues (model name and sensitive data logging) and considering the suggested improvements, this will be a robust solution.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/paths-filter.tsx (1)
Line range hint
13-89: Remove duplicate path entries in options arrayThe following paths are duplicated:
- "/v1/auth.login" (ids: 4, 10)
- "/v1/auth.logout" (ids: 5, 11)
- "/v1/auth.refreshToken" (ids: 6, 12)
- "/v1/data.delete" (ids: 7, 13)
- "/v1/data.fetch" (ids: 8, 14)
- "/v1/data.submit" (ids: 9, 15)
This could lead to confusion and potential bugs in filtering.
const options: CheckboxOption[] = [ { id: 1, path: "/v1/analytics.export", checked: false, }, { id: 2, path: "/v1/analytics.getDetails", checked: false, }, { id: 3, path: "/v1/analytics.getOverview", checked: false, }, { id: 4, path: "/v1/auth.login", checked: false, }, { id: 5, path: "/v1/auth.logout", checked: false, }, { id: 6, path: "/v1/auth.refreshToken", checked: false, }, { id: 7, path: "/v1/data.delete", checked: false, }, { id: 8, path: "/v1/data.fetch", checked: false, }, { id: 9, path: "/v1/data.submit", checked: false, }, - { - id: 10, - path: "/v1/auth.login", - checked: false, - }, - { - id: 11, - path: "/v1/auth.logout", - checked: false, - }, - { - id: 12, - path: "/v1/auth.refreshToken", - checked: false, - }, - { - id: 13, - path: "/v1/data.delete", - checked: false, - }, - { - id: 14, - path: "/v1/data.fetch", - checked: false, - }, - { - id: 15, - path: "/v1/data.submit", - checked: false, - }, ] as const;
🧹 Nitpick comments (8)
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-search/index.tsx (1)
75-84: Enhance accessibility with ARIA labels.Add appropriate ARIA labels to improve accessibility for screen readers.
<input ref={inputRef} type="text" value={searchText} onKeyDown={handleKeyDown} onChange={(e) => setSearchText(e.target.value)} placeholder="Search and filter with AI…" + aria-label="Search logs with AI" className="text-accent-12 font-medium text-[13px] bg-transparent border-none outline-none focus:ring-0 focus:outline-none placeholder:text-accent-12 selection:bg-gray-6" disabled={isLoading} /> <div> + <span className="sr-only">Search information</span> <CircleInfoSparkle className="size-4 text-accent-9" /> </div>Also applies to: 91-93
apps/dashboard/lib/trpc/routers/logs/llm-search.ts (1)
136-142: Add path format validation.Consider adding validation to ensure paths start with a forward slash when transforming filters.
case "methods": case "paths": case "status": if (filter.filters.length > 0) { + if (filter.field === "paths") { + // Ensure paths start with forward slash + filterValues.forEach(f => { + if (!f.value.startsWith("/")) { + f.value = `/${f.value}`; + } + }); + } output[filter.field] = filterValues; } break;internal/icons/src/index.ts (1)
30-31: Remove file extension from export path for consistency.The
.tsxextension in the export path is inconsistent with other exports in this file.-export * from "./icons/caret-right-outline.tsx"; +export * from "./icons/caret-right-outline";apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/hooks/use-checkbox-state.ts (1)
49-58: Consider memoizing handleCheckboxChange and handleSelectAll.The
handleTogglecallback depends on non-memoized functions, which could lead to unnecessary re-renders.+const handleCheckboxChange = useCallback((index: number): void => { setCheckboxes((prevCheckboxes) => { const newCheckboxes = [...prevCheckboxes]; newCheckboxes[index] = { ...newCheckboxes[index], checked: !newCheckboxes[index].checked, }; return newCheckboxes; }); -}; +}, []); +const handleSelectAll = useCallback((): void => { setCheckboxes((prevCheckboxes) => { const allChecked = prevCheckboxes.every((checkbox) => checkbox.checked); return prevCheckboxes.map((checkbox) => ({ ...checkbox, checked: !allChecked, })); }); -}; +}, []);apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/filter-checkbox.tsx (1)
46-58: Consider memoizing filter value creation.The filter value creation in
handleApplyFiltercould be optimized by memoizing theselectedValuestransformation.const handleApplyFilter = useCallback(() => { - const selectedValues = checkboxes.filter((c) => c.checked).map((c) => createFilterValue(c)); + const selectedValues = useMemo( + () => checkboxes.filter((c) => c.checked).map((c) => createFilterValue(c)), + [checkboxes, createFilterValue] + ); const otherFilters = filters.filter((f) => f.field !== filterField);apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/status-filter.tsx (1)
38-60: Simplify color logic in createFilterValue.The nested ternary expressions for color selection could be simplified for better readability.
createFilterValue={(option) => ({ value: option.status, metadata: { - colorClass: - option.status >= 500 - ? "bg-error-9" - : option.status >= 400 - ? "bg-warning-8" - : "bg-success-9", + colorClass: { + 500: "bg-error-9", + 400: "bg-warning-8", + 200: "bg-success-9", + }[Math.floor(option.status / 100) * 100] || "bg-success-9", }, })}apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/filters-popover.tsx (2)
42-43: Excellent keyboard navigation implementation with room for accessibility improvementsGreat job implementing comprehensive keyboard navigation with both arrow keys and vim-style shortcuts! To enhance accessibility, consider adding ARIA attributes for screen readers.
Add these ARIA attributes to improve screen reader support:
<PopoverContent className="w-60 bg-gray-1 dark:bg-black drop-shadow-2xl p-2 border-gray-6 rounded-lg" align="start" onKeyDown={handleKeyDown} + role="menu" + aria-label="Filter options" >For each filter item:
<div ref={itemRef} className={...} tabIndex={0} role="button" data-filter-id={id} + aria-haspopup="true" + aria-expanded={open} >Also applies to: 49-98
163-171: Use semantic HTML elements instead of ARIA rolesReplace the div with role="button" with a native button element for better accessibility and keyboard handling.
-<div +<button ref={itemRef} 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 ${isFocused ? "bg-gray-3" : ""}`} - tabIndex={0} - role="button" data-filter-id={id} -> +</button>🧰 Tools
🪛 Biome (1.9.4)
[error] 168-169: The elements with the following roles can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/dashboard/app/(app)/logs-v2/components/control-cloud/index.tsx(4 hunks)apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/filter-checkbox.tsx(1 hunks)apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/filters-popover.tsx(6 hunks)apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/hooks/use-checkbox-state.ts(1 hunks)apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/methods-filter.tsx(2 hunks)apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/paths-filter.tsx(4 hunks)apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/status-filter.tsx(2 hunks)apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-search/index.tsx(1 hunks)apps/dashboard/components/timestamp-info.tsx(1 hunks)apps/dashboard/lib/trpc/routers/logs/llm-search.ts(1 hunks)internal/icons/src/icons/caret-right-outline.tsx(1 hunks)internal/icons/src/icons/circle-info-sparkle.tsx(1 hunks)internal/icons/src/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/app/(app)/logs-v2/components/control-cloud/index.tsx
🧰 Additional context used
📓 Learnings (1)
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/methods-filter.tsx (1)
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2801
File: apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/methods-filter.tsx:39-62
Timestamp: 2025-01-10T10:09:42.433Z
Learning: In the logs-v2 filters implementation, handler improvements and type safety enhancements should be deferred until real data integration, as the handlers will need to be modified to work with the actual data source.
🪛 Biome (1.9.4)
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/paths-filter.tsx
[error] 139-154: 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] 139-145: The HTML element with the interactive role "checkbox" is not focusable.
A non-interactive HTML element that is not focusable may not be reachable for users that rely on keyboard navigation, even with an added role like "checkbox".
Add a tabIndex attribute to make this element focusable.
(lint/a11y/useFocusableInteractive)
[error] 140-142: The elements with the following roles can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
[error] 161-175: 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] 161-168: The HTML element with the interactive role "checkbox" is not focusable.
A non-interactive HTML element that is not focusable may not be reachable for users that rely on keyboard navigation, even with an added role like "checkbox".
Add a tabIndex attribute to make this element focusable.
(lint/a11y/useFocusableInteractive)
[error] 163-165: The elements with the following roles can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/filter-checkbox.tsx
[error] 71-87: 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] 71-77: The HTML element with the interactive role "checkbox" is not focusable.
A non-interactive HTML element that is not focusable may not be reachable for users that rely on keyboard navigation, even with an added role like "checkbox".
Add a tabIndex attribute to make this element focusable.
(lint/a11y/useFocusableInteractive)
[error] 72-74: The elements with the following roles can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
[error] 90-105: 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] 90-97: The HTML element with the interactive role "checkbox" is not focusable.
A non-interactive HTML element that is not focusable may not be reachable for users that rely on keyboard navigation, even with an added role like "checkbox".
Add a tabIndex attribute to make this element focusable.
(lint/a11y/useFocusableInteractive)
[error] 92-94: The elements with the following roles can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/filters-popover.tsx
[error] 168-169: The elements with the following roles can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
⏰ 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/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 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 (15)
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-search/index.tsx (1)
34-42: Enhance error handling in handleSearch.The catch block should handle the error more gracefully instead of just logging to console.
const handleSearch = async () => { if (searchText.trim()) { try { await queryLLMForStructuredQuery.mutateAsync(searchText); } catch (error) { - console.error("Search failed:", error); + // Error is already handled by the mutation's onError callback + // No need for additional console.error } } };apps/dashboard/lib/trpc/routers/logs/llm-search.ts (3)
25-25: Incorrect OpenAI model nameThe model name
"gpt-4o-2024-08-06"appears to be incorrect. Please verify and update it to the correct model name. Common model names are"gpt-4"or"gpt-3.5-turbo".-model: "gpt-4o-2024-08-06", +model: "gpt-4",
60-64: Avoid logging sensitive user inputs in error messagesLogging user inputs (
userSearchMsg) in error messages can lead to potential exposure of sensitive information. Please remove or obfuscate user inputs from log statements to prevent PII leakage.console.error( - `Something went wrong when querying OpenAI. Input: ${JSON.stringify( - userSearchMsg, - )}\n Output ${(error as Error).message}}`, + `Failed to query OpenAI API. Error: ${(error as Error).message}`, );
84-108: Well-implemented workspace access verification and rate limiting!The implementation includes:
- Proper rate limiting
- Thorough workspace access verification
- Helpful error messages with support contact
internal/icons/src/icons/caret-right-outline.tsx (2)
15-30: LGTM! Well-structured icon component.The component follows React best practices with proper typing and SVG attributes.
1-11: Verify compliance with Nucleo Icons license.The icon is from Nucleo Icons which has usage restrictions. Ensure we have proper licensing for using these icons in our product.
✅ Verification successful
License compliance confirmed for Nucleo Icons
The icons are properly licensed and used in compliance with Nucleo's terms. The LICENSE file and consistent copyright headers across all icon files confirm proper attribution and usage within the permitted bounds of bundling with the product.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if we have a valid Nucleo license file fd -g "*license*" -g "*nucleo*" -iLength of output: 132
Script:
#!/bin/bash # Search for license files and Nucleo references fd -t f "license" -i fd -t f "nucleo" -i # Check package.json files for Nucleo references fd -t f "package.json" --exec grep -i "nucleo" {} \; # Look for similar icon files to check for consistent licensing fd -t f -e tsx -e ts -p "internal/icons" --exec head -n 11 {} \;Length of output: 9954
Script:
#!/bin/bash # Check the content of the icons license file cat internal/icons/LICENSELength of output: 285
internal/icons/src/icons/circle-info-sparkle.tsx (1)
15-51: LGTM! Well-implemented icon component.The SVG implementation is correct with proper accessibility attributes and styling.
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/hooks/use-checkbox-state.ts (2)
4-9: LGTM! Well-structured type definitions.The generic type implementation with proper constraints and clear interface definition enhances type safety and reusability.
17-26: Consider adding error handling for invalid filter values.The initial state setup assumes all filter values can be converted to strings safely.
apps/dashboard/components/timestamp-info.tsx (1)
22-22: LGTM! Format string improvements for better readability.The changes to use "MMM dd HH:mm:ss" format:
- Removes milliseconds for cleaner display
- Uses HH to ensure consistent 24-hour format with leading zeros
Also applies to: 29-29
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/filter-checkbox.tsx (1)
9-25: LGTM! Well-structured type definitions and props interface.The
BaseCheckboxOptiontype andBaseCheckboxFilterPropsinterface provide good type safety and reusability.apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/methods-filter.tsx (1)
20-30: LGTM! Clean implementation using FilterCheckbox.The refactoring aligns with the retrieved learning about deferring handler improvements until real data integration.
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/paths-filter.tsx (2)
1-2: Well-structured code organization!Good job separating types, hooks, and components into dedicated files. The introduction of
useCheckboxStatehook promotes reusability and maintainability.Also applies to: 6-6
Line range hint
119-134: Well-implemented filter application logic!The implementation correctly:
- Preserves existing non-path filters
- Generates unique IDs for new filters
- Maps selected paths to filter values
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-filters/components/filters-popover.tsx (1)
Line range hint
184-190: Well-designed filter status indicator!Good implementation of the filter count indicator with:
- Clear visual feedback
- Good contrast ratio
- Appropriate sizing and positioning
apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-search/index.tsx
Show resolved
Hide resolved
...)/logs-v2/components/controls/components/logs-filters/components/hooks/use-checkbox-state.ts
Show resolved
Hide resolved
...app/(app)/logs-v2/components/controls/components/logs-filters/components/filter-checkbox.tsx
Show resolved
Hide resolved
...rd/app/(app)/logs-v2/components/controls/components/logs-filters/components/paths-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
Based on the comprehensive changes across multiple files, here are the updated release notes:
New Features
FilterCheckboxcomponent for managing checkbox filters.Improvements
Technical Enhancements
Dependencies