Conversation
ogzhanolguncu
requested review from
chronark,
mcstepp and
perkinsjr
as code owners
|
📝 WalkthroughWalkthroughThis pull request introduces extensive restructuring and enhancements across the audit logging system. The changes integrate organization member retrieval into workspace fetching, add several new React components for audit log controls (such as filters, search, date-time, and refresh), update table rendering with new types and hooks, refactor TRPC procedures to support structured audit log queries and LLM-based search, and remove deprecated components and types. Imports, constants, and utility functions have also been updated or relocated to support these modifications. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Audit Client
participant WorkspaceFn as getWorkspace()
participant Clerk as clerkClient
participant Logger as Error Logger
Client->>WorkspaceFn: Call getWorkspace(tenantId,...)
WorkspaceFn->>WorkspaceFn: Check if tenantId starts with "user_"
alt Organization Tenant
WorkspaceFn->>Clerk: Call clerkClient.organizations.getOrganizationMembershipList
Clerk-->>WorkspaceFn: Return membership list
WorkspaceFn->>WorkspaceFn: Filter members with publicUserData
else User Tenant
WorkspaceFn->>WorkspaceFn: Set members as null
end
WorkspaceFn->>Client: Return { workspace, members }
Note over WorkspaceFn: Error handling logs issues via Logger
sequenceDiagram
participant User as End User
participant SearchComp as LogsSearch Component
participant TRPC as TRPC Mutation (queryLLMForStructuredOutput)
participant LLM as OpenAI API
participant Filters as Filter Update Handler
User->>SearchComp: Submit search query
SearchComp->>TRPC: Trigger mutation with query and timestamp
TRPC->>LLM: Call getStructuredAuditSearchFromLLM(query, timestamp)
LLM-->>TRPC: Return structured audit search result
TRPC->>SearchComp: Return response (filters or error)
alt Successful Response
SearchComp->>Filters: Update filters with structured output
else Error Response
SearchComp->>User: Display error toast
end
Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command ✨ 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 (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
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: 7
🧹 Nitpick comments (26)
apps/dashboard/app/(app)/audit/components/control-cloud/index.tsx (1)
26-37: Add error handling for possible undefined filter values.The component utilizes values from the
useFiltershook without checking if they're undefined. Consider adding default values or null checks to prevent potential runtime errors.export const AuditLogsControlCloud = () => { - const { filters, updateFilters, removeFilter } = useFilters(); + const { filters = {}, updateFilters = () => {}, removeFilter = () => {} } = useFilters(); return ( <ControlCloud historicalWindow={HISTORICAL_DATA_WINDOW} formatFieldName={formatFieldName} filters={filters} removeFilter={removeFilter} updateFilters={updateFilters} /> ); };apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/components/events-filter.tsx (1)
14-19: Add error handling for empty events array.The component doesn't handle the case where
unkeyAuditLogEvents.Valuesmight be undefined or empty. Consider adding a fallback or check to prevent potential errors.options={Object.values(unkeyAuditLogEvents.Values).map<EventsOption>((value, index) => ({ id: index, display: value, label: value, checked: false, - }))} + })) || []}apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/components/root-keys-filter.tsx (1)
14-19: Defensive null check is redundant with TypeScript definition.The code includes a null check
(rootKeys ?? [])but the prop type doesn't allow for null. Either update the type to includenullor remove the null check.- options={(rootKeys ?? []).map((rootKey, index) => ({ + options={rootKeys.map((rootKey, index) => ({Or update the type definition to:
rootKeys: { id: string; name: string | null; }[] | null;apps/dashboard/lib/trpc/routers/audit/llm-search/index.ts (1)
15-39: Consider adding specific error handling for OpenAI API errors.The implementation calls
getStructuredAuditSearchFromLLMbut doesn't provide specific error handling for potential OpenAI API errors (rate limiting, token limits, etc.).Consider wrapping the OpenAI call with specific error handling:
- return await getStructuredAuditSearchFromLLM(openai, input.query, input.timestamp); + try { + return await getStructuredAuditSearchFromLLM(openai, input.query, input.timestamp); + } catch (error) { + if (error instanceof OpenAI.APIError) { + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: `OpenAI API error: ${error.message}`, + cause: error, + }); + } + throw error; + }apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/components/bucket-filter.tsx (1)
1-30: Add better handling for empty bucket filters.The component correctly handles null
bucketFilterwith the nullish coalescing operator, but could improve the user experience when there are no options.Consider enhancing the component with empty state handling:
export const BucketFilter = ({ bucketFilter, }: { bucketFilter: { id: string; name: string | null; }[]; }) => { + const options = (bucketFilter ?? []).map((rootKey, index) => ({ + label: rootKey.name, + value: rootKey.id, + checked: false, + id: index, + })); + + if (options.length === 0) { + return <div className="text-muted-foreground text-xs py-2">No bucket filters available</div>; + } return ( <FilterCheckbox showScroll - options={(bucketFilter ?? []).map((rootKey, index) => ({ - label: rootKey.name, - value: rootKey.id, - checked: false, - id: index, - }))} + options={options} filterField="status" checkPath="status" renderOptionContent={(checkbox) => ( <div className="text-accent-12 text-xs">{checkbox.label}</div> )} createFilterValue={(option) => ({ value: option.value, })} /> ); };apps/dashboard/app/(app)/audit/components/controls/components/logs-search/index.tsx (1)
7-56: Consider debouncing search requests.There's no mechanism to prevent rapid successive search requests, which could lead to rate limiting or unnecessary API calls.
Add debouncing to the search function:
import { LogsLLMSearch } from "@/components/logs/llm-search"; import { transformStructuredOutputToFilters } from "@/components/logs/validation/utils/transform-structured-output-filter-format"; import { toast } from "@/components/ui/toaster"; import { trpc } from "@/lib/trpc/client"; +import { useDebouncedCallback } from "use-debounce"; import { useFilters } from "../../../../hooks/use-filters"; export const LogsSearch = () => { const { filters, updateFilters } = useFilters(); const queryLLMForStructuredOutput = trpc.audit.llmSearch.useMutation({ // ... existing code ... }); + const debouncedSearch = useDebouncedCallback((query: string) => { + queryLLMForStructuredOutput.mutateAsync({ + query, + timestamp: Date.now(), + }); + }, 500); return ( <LogsLLMSearch isLoading={queryLLMForStructuredOutput.isLoading} - onSearch={(query) => - queryLLMForStructuredOutput.mutateAsync({ - query, - timestamp: Date.now(), - }) - } + onSearch={debouncedSearch} /> ); };apps/dashboard/lib/trpc/routers/audit/utils.ts (1)
16-32: Consistent filter mapping implementationThe implementation for mapping events, users, and rootKeys filters follows the same pattern, which is good for consistency and maintainability. However, consider using a helper function to reduce repetition.
+// Helper function to map filters +const mapFilters = (filtersObj?: { filters: Array<{ operator: string; value: string }> }) => + filtersObj?.filters.map((f) => ({ + operator: f.operator, + value: f.value, + })) ?? []; + export function transformFilters( params: AuditQueryLogsPayload, ): Omit<AuditQueryLogsParams, "workspaceId"> { // ... existing code ... - const events = - params.events?.filters.map((f) => ({ - operator: f.operator, - value: f.value, - })) ?? []; - - const users = - params.users?.filters.map((f) => ({ - operator: f.operator, - value: f.value, - })) ?? []; - - const rootKeys = - params.rootKeys?.filters.map((f) => ({ - operator: f.operator, - value: f.value, - })) ?? []; + const events = mapFilters(params.events); + const users = mapFilters(params.users); + const rootKeys = mapFilters(params.rootKeys);apps/dashboard/app/(app)/audit/components/table/query-logs.schema.ts (1)
10-19: Repeated filter structure patternThe schema definition for events, users, and rootKeys follows the same pattern. Consider extracting a reusable filter schema to reduce duplication.
+const filterSchema = z.object({ + filters: z.array( + z.object({ + operator: z.literal("is"), + value: z.string(), + }), + ), +}); + export const auditQueryLogsPayload = z.object({ limit: z.number().int(), startTime: z.number().int(), endTime: z.number().int(), since: z.string(), bucket: z.string().default(DEFAULT_BUCKET_NAME), - events: z - .object({ - filters: z.array( - z.object({ - operator: z.literal("is"), - value: z.string(), - }), - ), - }) - .nullable(), - users: z - .object({ - filters: z.array( - z.object({ - operator: z.literal("is"), - value: z.string(), - }), - ), - }) - .nullable(), - rootKeys: z - .object({ - filters: z.array( - z.object({ - operator: z.literal("is"), - value: z.string(), - }), - ), - }) - .nullable(), + events: filterSchema.nullable(), + users: filterSchema.nullable(), + rootKeys: filterSchema.nullable(),Also applies to: 20-29, 30-39
apps/dashboard/app/(app)/audit/components/controls/components/logs-datetime/index.tsx (1)
9-16: Simplify state initializationThe
useEffecthook is being used to set a default value for thetitlestate when it could be done directly in theuseStatecall. This would simplify the code and avoid an unnecessary effect run.- const [title, setTitle] = useState<string | null>(null); + const [title, setTitle] = useState<string>("Last 12 hours"); - useEffect(() => { - if (!title) { - setTitle("Last 12 hours"); - } - }, [title]);apps/dashboard/app/(app)/audit/components/table/logs-table.tsx (2)
78-96: Extract actor rendering logic to a separate functionThe actor rendering logic is complex with multiple conditions. Consider extracting this to a separate function to improve readability and maintainability.
// Add this function above the columns definition +const renderActor = (log: AuditLog) => { + if (log.auditLog.actor.type === "user" && log.user) { + return ( + <div className="flex items-center w-full gap-2 max-sm:m-0 max-sm:gap-1 max-sm:text-xs"> + <span className="text-xs whitespace-nowrap"> + {`${log.user.firstName ?? ""} ${log.user.lastName ?? ""}`} + </span> + </div> + ); + } else if (log.auditLog.actor.type === "key") { + return ( + <div className="flex items-center w-full gap-2 max-sm:m-0 max-sm:gap-1 max-sm:text-xs"> + <KeySquare className="w-4 h-4" /> + <span className="font-mono text-xs truncate">{log.auditLog.actor.id}</span> + </div> + ); + } else { + return ( + <div className="flex items-center w-full gap-2 max-sm:m-0 max-sm:gap-1 max-sm:text-xs"> + <FunctionSquare className="w-4 h-4" /> + <span className="font-mono text-xs truncate">{log.auditLog.actor.id}</span> + </div> + ); + } +}; // Then use it in the column definition render: (log) => ( <div className="flex items-center gap-3 truncate"> - {log.auditLog.actor.type === "user" && log.user ? ( - <div className="flex items-center w-full gap-2 max-sm:m-0 max-sm:gap-1 max-sm:text-xs"> - <span className="text-xs whitespace-nowrap"> - {`${log.user.firstName ?? ""} ${log.user.lastName ?? ""}`} - </span> - </div> - ) : log.auditLog.actor.type === "key" ? ( - <div className="flex items-center w-full gap-2 max-sm:m-0 max-sm:gap-1 max-sm:text-xs"> - <KeySquare className="w-4 h-4" /> - <span className="font-mono text-xs truncate">{log.auditLog.actor.id}</span> - </div> - ) : ( - <div className="flex items-center w-full gap-2 max-sm:m-0 max-sm:gap-1 max-sm:text-xs"> - <FunctionSquare className="w-4 h-4" /> - <span className="font-mono text-xs truncate">{log.auditLog.actor.id}</span> - </div> - )} + {renderActor(log)} </div> ),
136-138: Consider using text overflow styling instead of fixed widthThe description column has a fixed width of 200px which might truncate longer descriptions. Consider using CSS text overflow properties instead of a fixed width to allow the description to take up available space while still truncating when necessary.
render: (log) => ( - <div className="font-mono text-xs truncate w-[200px]">{log.auditLog.description}</div> + <div className="font-mono text-xs truncate max-w-full">{log.auditLog.description}</div> ),apps/dashboard/app/(app)/audit/hooks/use-filters.ts (2)
70-80: Use type guards for safer type castingThe code uses type casting for fields and values which could lead to runtime errors. Consider using type guards for safer type handling.
["startTime", "endTime", "since"].forEach((field) => { const value = searchParams[field as keyof QuerySearchParams]; if (value !== null && value !== undefined) { + // Check if field is a valid AuditLogsFilterField + if (!["startTime", "endTime", "since"].includes(field)) { + console.error(`Invalid field: ${field}`); + return; + } activeFilters.push({ - field: field as AuditLogsFilterField, + field: field as AuditLogsFilterField, // Now safer with validation operator: "is", - value: value as string | number, + value: value, }); } });
102-133: Add a default case to the switch statementThe switch statement doesn't have a default case to handle unexpected filter field values. This could lead to silent failures if a new filter field is added but the switch statement isn't updated.
newFilters.forEach((filter) => { switch (filter.field) { case "events": // existing code... break; // other cases... case "bucket": newParams.bucket = filter.value as string; break; case "startTime": case "endTime": newParams[filter.field] = filter.value as number; break; case "since": newParams.since = filter.value as string; break; + default: + console.warn(`Unhandled filter field: ${filter.field}`); + break; } });apps/dashboard/app/(app)/audit/components/table/hooks/use-logs-query.ts (2)
73-73: Fix incorrect error messageThe error message for startTime/endTime filter says the value type should be 'string', but the type check is for 'number'.
- console.error(`${filter.field} filter value type has to be 'string'`); + console.error(`${filter.field} filter value type has to be 'number'`);
33-98: Add a default case to the switch statementThe switch statement doesn't handle unexpected filter field values. Adding a default case would make the code more robust.
switch (filter.field) { case "events": { // existing code... } // other cases... case "bucket": { // existing code... break; } + default: { + console.warn(`Unhandled filter field: ${filter.field}`); + break; + } }apps/dashboard/app/(app)/audit/actions.ts (2)
7-30: Handle potential partial failure more explicitly.When membership fetching fails (lines 23–29), only a console error is logged, and the code continues with
members = null. This can be acceptable, but returning a partial or null membership list might confuse downstream logic. Consider returning an empty array instead ofnullfor better consistency.- let members = null; + let members: { label: string; value: string }[] = [];
56-65: Return structure looks coherent, but ensure consistent handling formembers.Appending
keysto the workspace object is well-organized. However, consider returning[]instead ofnullformembersto avoid null-check overhead in the consumer.apps/dashboard/app/(app)/audit/components/logs-client.tsx (2)
10-25: Prefer using an empty array formembersinstead ofnull.Defining
membersas| nullcan force consumers to do null checks. Consider always returning a non-null, possibly empty, array for simpler handling.
37-43: Spreading props is convenient but may pass unneeded fields.
<AuditLogsControls {...props} />ensures the component has everything it needs. This is fine as a short-term solution, but if the prop set grows, consider passing only relevant fields explicitly for clarity.apps/dashboard/lib/trpc/routers/audit/llm-search/utils.ts (1)
82-259: Large inline prompt is fine but may be extracted into a config for maintainability.
getAuditSystemPromptis well-documented. For large strings, consider placing them in a separate config or storing them as template files to simplify future changes.apps/dashboard/lib/trpc/routers/audit/schema.ts (1)
4-35: Ensure consistent usage of user object
You may consider providing a default user object or ensuring the user property is always defined. This can simplify consumer code and reduce null checks at runtime.apps/dashboard/app/(app)/audit/components/table/utils/get-row-class.ts (2)
7-27: Consider broader coverage of synonyms
getEventTypehandles "create", "update", and "delete" with synonyms like "add", "edit", and "remove". Consider including other potential synonyms ("rename", "archive", etc.) if needed later.
87-103: Refine row selection styling
The current approach toggles opacity whenlogSelectedis true butisSelectedis false. Consider a more distinct visual indicator for clarity.apps/dashboard/app/(app)/audit/filters.schema.ts (1)
8-37: Operator variety
Currently, only the "is" operator is supported. In future, you might add "contains", "in", or other operators to expand filter capabilities.apps/dashboard/lib/trpc/routers/audit/fetch.ts (2)
87-135: Potential indexing for performance
The approach to filtering byactorIdandeventis solid. Depending on volume, adding database indexes could boost query performance for large datasets.
136-142: Simple pagination logic
omitLastItemForPaginationis straightforward and effective for detectinghasMore. Just confirm the UI handles the final page properly (e.g., no extra empty state).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (53)
apps/dashboard/app/(app)/audit/actions.ts(2 hunks)apps/dashboard/app/(app)/audit/audit.type.ts(0 hunks)apps/dashboard/app/(app)/audit/components/control-cloud/index.tsx(1 hunks)apps/dashboard/app/(app)/audit/components/controls/components/logs-datetime/index.tsx(1 hunks)apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/components/bucket-filter.tsx(1 hunks)apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/components/events-filter.tsx(1 hunks)apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/components/root-keys-filter.tsx(1 hunks)apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/components/users-filter.tsx(1 hunks)apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/index.tsx(1 hunks)apps/dashboard/app/(app)/audit/components/controls/components/logs-refresh.tsx(1 hunks)apps/dashboard/app/(app)/audit/components/controls/components/logs-search/index.tsx(1 hunks)apps/dashboard/app/(app)/audit/components/controls/index.tsx(1 hunks)apps/dashboard/app/(app)/audit/components/filters/bucket-select.tsx(0 hunks)apps/dashboard/app/(app)/audit/components/filters/clear-button.tsx(0 hunks)apps/dashboard/app/(app)/audit/components/filters/datepicker-with-range.tsx(0 hunks)apps/dashboard/app/(app)/audit/components/filters/filter.tsx(0 hunks)apps/dashboard/app/(app)/audit/components/filters/index.tsx(0 hunks)apps/dashboard/app/(app)/audit/components/filters/root-key-filter.tsx(0 hunks)apps/dashboard/app/(app)/audit/components/filters/timesplit-input.tsx(0 hunks)apps/dashboard/app/(app)/audit/components/filters/user-filter.tsx(0 hunks)apps/dashboard/app/(app)/audit/components/logs-client.tsx(1 hunks)apps/dashboard/app/(app)/audit/components/table/audit-logs-table.tsx(0 hunks)apps/dashboard/app/(app)/audit/components/table/columns.tsx(0 hunks)apps/dashboard/app/(app)/audit/components/table/constants.ts(0 hunks)apps/dashboard/app/(app)/audit/components/table/hooks/use-logs-query.ts(1 hunks)apps/dashboard/app/(app)/audit/components/table/log-details/components/log-footer.tsx(1 hunks)apps/dashboard/app/(app)/audit/components/table/log-details/components/log-header.tsx(1 hunks)apps/dashboard/app/(app)/audit/components/table/log-details/index.tsx(2 hunks)apps/dashboard/app/(app)/audit/components/table/log-details/resizable-panel.tsx(0 hunks)apps/dashboard/app/(app)/audit/components/table/logs-table.tsx(1 hunks)apps/dashboard/app/(app)/audit/components/table/query-logs.schema.ts(1 hunks)apps/dashboard/app/(app)/audit/components/table/utils.ts(0 hunks)apps/dashboard/app/(app)/audit/components/table/utils/get-row-class.ts(1 hunks)apps/dashboard/app/(app)/audit/filters.schema.ts(1 hunks)apps/dashboard/app/(app)/audit/hooks/use-filters.ts(1 hunks)apps/dashboard/app/(app)/audit/page.tsx(2 hunks)apps/dashboard/app/(app)/audit/query-state.ts(0 hunks)apps/dashboard/app/(app)/logs/components/table/log-details/components/log-footer.tsx(2 hunks)apps/dashboard/app/(app)/logs/components/table/log-details/index.tsx(1 hunks)apps/dashboard/app/(app)/logs/constants.ts(0 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/request-response-details.tsx(0 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/log-details/index.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/log-details/resizable-panel.tsx(0 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/constants.ts(0 hunks)apps/dashboard/components/logs/details/resizable-panel.tsx(1 hunks)apps/dashboard/components/virtual-table/index.tsx(2 hunks)apps/dashboard/lib/trpc/routers/audit/fetch.ts(3 hunks)apps/dashboard/lib/trpc/routers/audit/llm-search/index.ts(1 hunks)apps/dashboard/lib/trpc/routers/audit/llm-search/utils.ts(1 hunks)apps/dashboard/lib/trpc/routers/audit/schema.ts(1 hunks)apps/dashboard/lib/trpc/routers/audit/utils.ts(1 hunks)apps/dashboard/lib/trpc/routers/index.ts(2 hunks)
💤 Files with no reviewable changes (19)
- apps/dashboard/app/(app)/audit/components/table/utils.ts
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/constants.ts
- apps/dashboard/app/(app)/audit/components/filters/filter.tsx
- apps/dashboard/app/(app)/audit/components/filters/root-key-filter.tsx
- apps/dashboard/app/(app)/audit/components/filters/bucket-select.tsx
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/log-details/resizable-panel.tsx
- apps/dashboard/app/(app)/audit/components/filters/user-filter.tsx
- apps/dashboard/app/(app)/audit/audit.type.ts
- apps/dashboard/app/(app)/audit/components/table/constants.ts
- apps/dashboard/app/(app)/audit/components/filters/index.tsx
- apps/dashboard/app/(app)/audit/components/filters/clear-button.tsx
- apps/dashboard/app/(app)/audit/components/table/columns.tsx
- apps/dashboard/app/(app)/audit/components/filters/timesplit-input.tsx
- apps/dashboard/app/(app)/audit/components/table/log-details/resizable-panel.tsx
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/log-details/components/request-response-details.tsx
- apps/dashboard/app/(app)/audit/components/filters/datepicker-with-range.tsx
- apps/dashboard/app/(app)/logs/constants.ts
- apps/dashboard/app/(app)/audit/query-state.ts
- apps/dashboard/app/(app)/audit/components/table/audit-logs-table.tsx
✅ Files skipped from review due to trivial changes (2)
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/log-details/components/log-footer.tsx
- apps/dashboard/components/logs/details/resizable-panel.tsx
🧰 Additional context used
🧠 Learnings (2)
apps/dashboard/app/(app)/audit/components/controls/components/logs-datetime/index.tsx (1)
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2825
File: apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-datetime/index.tsx:0-0
Timestamp: 2025-01-30T20:38:00.058Z
Learning: In the logs dashboard, keyboard shortcuts that toggle UI elements (like popovers) should be implemented in the component that owns the state being toggled, not in the presentational wrapper components. For example, the 'T' shortcut for toggling the datetime filter is implemented in DatetimePopover, not in LogsDateTime.
apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/index.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.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- 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: Analyze (javascript-typescript)
🔇 Additional comments (62)
apps/dashboard/components/virtual-table/index.tsx (7)
3-3: Updated imports to support ref forwardingThe import statement has been modified to include React features needed for forwarding refs:
Ref,forwardRef, anduseImperativeHandle. These are necessary for the component refactoring to expose internal references to parent components.
30-33: Good type definition for exposed refsThe
VirtualTableReftype clearly defines what internal references will be exposed to parent components, making the component API more explicit and type-safe.
35-53: Well-implemented forwardRef patternThe component has been properly converted to use the
forwardRefpattern, which allows parent components to access internal references. The function signature maintains the same props interface while adding ref forwarding capabilities.
70-78: Correct implementation of useImperativeHandleThe
useImperativeHandlehook is well-implemented to expose the internal refs to parent components. The empty dependency array is appropriate since the refs themselves don't change.
82-118: Preserved empty state rendering logicThe empty state rendering is properly maintained in the refactored component, ensuring consistent behavior when there's no data to display. The component correctly references the new refs.
120-282: Maintained component functionality with ref integrationThe main rendering logic has been preserved while integrating the new ref functionality. The virtual table implementation maintains all its previous capabilities including:
- Virtual row rendering
- Keyboard navigation
- Row selection
- Loading states
The structure adjustments support the ref forwarding without changing the component's core behavior.
283-284: Properly closed the forwardRef componentThe component declaration is correctly closed with the appropriate syntax for forwardRef components.
apps/dashboard/app/(app)/logs/components/table/log-details/index.tsx (2)
2-2: Good move centralizing the ResizablePanel component importMoving the import from a local path to a shared component path (
@/components/logs/details/resizable-panel) improves component reusability across the application.
35-40:Details
✅ Verification successful
Verify ResizablePanel props after moving the component
The ResizablePanel component is now being used without minW/maxW props that were previously set with PANEL_MIN_WIDTH and PANEL_MAX_WIDTH constants. Ensure that the central component still handles width constraints appropriately.
🏁 Script executed:
#!/bin/bash # Check if the centralized ResizablePanel component has width constraint props grep -A 10 "export.*ResizablePanel" apps/dashboard/components/logs/details/resizable-panel.tsx || echo "File not found"Length of output: 333
Width constraints are properly applied
The centralizedResizablePanelnow includesminWandmaxWprops with defaults set toMIN_DRAGGABLE_WIDTHandMAX_DRAGGABLE_WIDTH, ensuring that width constraints are still enforced. No further modifications are required in the logs component.apps/dashboard/app/(app)/audit/components/controls/index.tsx (1)
1-29: Well-structured control component with good separation of concernsThis new
AuditLogsControlscomponent nicely organizes all the audit log filtering controls in a clean, structured layout. The component follows good practices:
- Proper type usage with WorkspaceProps
- Logical grouping of related controls
- Clean flexbox layout with appropriate spacing
- Props correctly passed to the LogsFilters component
The structure makes the UI more maintainable and provides a consistent user experience.
apps/dashboard/app/(app)/audit/components/controls/components/logs-refresh.tsx (1)
5-14: Good implementation with conditional enablementThe LogsRefresh component is well-implemented with:
- Appropriate use of the useFilters hook to access filter state
- Smart conditional enabling of the refresh button based on filter presence
- Clean integration with TRPC for cache invalidation
The decision to only enable refresh when a relative "since" filter exists is a good UX choice that prevents unnecessary refreshes.
apps/dashboard/app/(app)/audit/components/table/log-details/components/log-header.tsx (2)
2-2: Type update ensures consistency across componentsUpdating the log prop type from
AuditDatatoAuditLogstandardizes the log data structure across components, improving type safety and reducing potential errors.Also applies to: 7-7
15-15: Badge styling update for improved visual hierarchyThe Badge component styling has been updated from
bg-accent-3/hover:bg-accent-4tobg-accent-4/hover:bg-accent-5, which slightly darkens the appearance. This likely improves visual contrast for better readability.apps/dashboard/lib/trpc/routers/index.ts (1)
143-144:Details
❓ Verification inconclusive
Verify existing references to the renamed audit router method.
The
fetchmethod has been renamed tologs, which is a breaking change for any code that references the old method name. Ensure all consumers of this API are updated accordingly.
🏁 Script executed:
#!/bin/bash # Search for any references to the old method name that might need updating echo "Searching for references to audit.fetch that need to be updated to audit.logs..." rg -A 2 -B 2 "audit\.fetch" --type-not testLength of output: 245
Ensure API references are updated for the renamed audit router method
The
fetchmethod in the audit router was renamed tologs—a breaking change that may affect any consumers still referencing the old name. Please verify that there are no remaining occurrences ofaudit.fetchin the codebase (using the revised search script above). If any are found, update them to useaudit.logs.
🏁 Script executed:
[run_scripts] #!/bin/bash echo "Searching for references to audit.fetch that need to be updated to audit.logs..." rg -A 2 -B 2 "audit\.fetch"Length of output: 322
Breaking API Change – Verify Consumer Updates for Renamed Audit Method
The
fetchmethod in the audit router has been renamed tologs, and a newllmSearchmethod has been added. This change is potentially breaking since consumers referencing the oldfetchmethod must now switch tologs. An automated search for occurrences ofaudit.fetchusingrgdid not reveal any matches. However, please verify manually that no consumers still reference the deprecated method.
- File:
apps/dashboard/lib/trpc/routers/index.ts(Lines 143-144)
- Old API:
fetch: fetchAuditLog- New API:
logs: fetchAuditLog, llmSearch: auditLogsSearch,Please double-check your local environment for any lingering references to
audit.fetchand update them toaudit.logsto ensure full compatibility.apps/dashboard/app/(app)/audit/components/control-cloud/index.tsx (1)
5-24: Well-structured field name formatter.The
formatFieldNamefunction is well implemented with proper handling for specific fields and a sensible default case that capitalizes the first letter. This improves the user experience by showing user-friendly labels.apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/components/events-filter.tsx (1)
13-21: Review field name consistency in filter configuration.There appears to be a mismatch between the data being filtered and the filter field name. The component maps over event values from
unkeyAuditLogEvents.Values, but setsfilterFieldandcheckPathto "status". Consider using "events" instead to maintain consistency with the data structure.options={Object.values(unkeyAuditLogEvents.Values).map<EventsOption>((value, index) => ({ id: index, display: value, label: value, checked: false, }))} - filterField="status" - checkPath="status" + filterField="events" + checkPath="events"apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/components/users-filter.tsx (2)
16-20: Good null handling for users prop.The component properly handles the case where the
usersprop isnullby defaulting to an empty array. This prevents runtime errors when mapping over potentially undefined values.
21-22: Review field name consistency in filter configuration.There appears to be a mismatch between the data being filtered and the filter field name. The component filters user data but sets
filterFieldandcheckPathto "status". Consider using "users" instead to maintain consistency with the data structure.- filterField="status" - checkPath="status" + filterField="users" + checkPath="users"apps/dashboard/app/(app)/audit/components/table/log-details/components/log-footer.tsx (2)
3-6: Type import refactor looks good.The change from
AuditDatatoAuditLogaligns with the broader refactoring of audit logs across the system. The import paths have been updated appropriately.
9-11: Type definition update is consistent.The
Propstype has been properly updated to use the newAuditLogtype, ensuring consistency with the rest of the audit components.apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/components/root-keys-filter.tsx (2)
1-2: Import looks good.The import of the
FilterCheckboxcomponent is correctly defined.
3-10: Props and types are well-defined.The component has clear and descriptive prop types, which will help with type safety during usage.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/log-details/index.tsx (2)
4-4: ResizablePanel import path updated correctly.The import path for
ResizablePanelhas been updated to use a centralized component path, which improves component reusability.
37-41:Details
❓ Verification inconclusive
ResizablePanel usage simplified.
The removal of
minWandmaxWprops from theResizablePanelindicates a move toward a more standardized implementation. This change should be verified to ensure the panel still resizes appropriately.
🏁 Script executed:
#!/bin/bash # Description: Check if ResizablePanel interface has changed to remove minW and maxW props # Look for ResizablePanel props type definition ast-grep --pattern 'type $ResizablePanelProps = { $$$ }' # Look for ResizablePanel component implementation ast-grep --pattern 'export const ResizablePanel = ({ $$$, minW, $$$, maxW, $$ }: $ResizablePanelProps) => { $$$ }' # Look for other usages of ResizablePanel to confirm consistency rg "ResizablePanel" --type tsx -A 2 -B 2 | grep -E "minW|maxW"Length of output: 270
Below is an updated shell script that narrows the search to the specific file and uses explicit globbing (via direct file paths) to check if there are any occurrences of
"minW"or"maxW"in the file. This will help verify that the new, simplified usage ofResizablePanelno longer includes these props.Please run the following script:
#!/bin/bash TARGET_FILE="apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/log-details/index.tsx" echo "Checking for 'minW' occurrences in $TARGET_FILE:" rg -o "minW" "$TARGET_FILE" echo "Checking for 'maxW' occurrences in $TARGET_FILE:" rg -o "maxW" "$TARGET_FILE" echo "Showing context around ResizablePanel usage in $TARGET_FILE:" rg "ResizablePanel" "$TARGET_FILE" -A 5 -B 5If this script confirms that neither
"minW"nor"maxW"is present and the context aroundResizablePanelis correct, then the removal of those props is validated.apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/index.tsx (3)
12-66: LogsFilters component structure looks good.The component is well-structured and follows a consistent pattern with other filter components in the system. The FiltersPopover implementation with proper keyboard shortcuts enhances accessibility.
Based on the retrieved learning from PR #2801, it's appropriate to defer handler improvements and type safety enhancements until real data integration is complete.
47-50: Active filter state UI enhancement is well implemented.The conditional styling based on filter state provides clear visual feedback to users when filters are active.
57-61: Filter count badge is a good UX feature.Displaying the count of active filters provides users with immediate feedback about their current filtering state.
apps/dashboard/app/(app)/logs/components/table/log-details/components/log-footer.tsx (2)
4-4: LGTM! Import path updated to absolute path.The import for
RequestResponseDetailswas updated to use an absolute path, which is consistent with the project's architecture.
52-52: Text alignment improved for user agent display.Adding
text-rightclass to the user agent display improves the UI layout when displaying long user agent strings.apps/dashboard/app/(app)/audit/components/table/log-details/index.tsx (4)
3-4: Import path updates for better organizationThe import paths have been updated to reflect a more structured component organization. The
ResizablePanelis now imported from a dedicated logs components directory, and theAuditLogtype is imported instead of the previousAuditDatatype.
6-6: Improved constants managementThe component now imports the
DEFAULT_DRAGGABLE_WIDTHconstant from a centralized location instead of using local constants like the removedPANEL_MAX_WIDTHandPANEL_MIN_WIDTH.
20-21: Type update for better type safetyThe component now properly uses the
AuditLogtype for the selected log prop, providing better type safety and consistency with the rest of the application.
36-40:Details
✅ Verification successful
Verify the panel's resizing behavior
The
ResizablePanelcomponent no longer receives theminWandmaxWprops that were previously set using the removed constants. Ensure this change doesn't affect the panel's resizing behavior.
🏁 Script executed:
#!/bin/bash # Check if the ResizablePanel component in the new location has default min and max width values grep -A 15 "export const ResizablePanel" apps/dashboard/components/logs/details/resizable-panel.tsxLength of output: 436
Verify Panel’s Resizing Behavior with Internal Defaults
The
ResizablePanelcomponent now defines default values forminWandmaxW(usingMIN_DRAGGABLE_WIDTHandMAX_DRAGGABLE_WIDTHrespectively) within its implementation inapps/dashboard/components/logs/details/resizable-panel.tsx. This means that even though the parent component inapps/dashboard/app/(app)/audit/components/table/log-details/index.tsxno longer passes these props explicitly, the panel’s resizing behavior should remain consistent.
- Confirm that the default width constraints meet the design requirements.
- If any unexpected resizing behavior is observed, consider revisiting the default constants in the
ResizablePanelcomponent.apps/dashboard/lib/trpc/routers/audit/utils.ts (2)
5-45: Well-structured filter transformation functionThe new
transformFiltersfunction cleanly separates the concern of transforming client-side filter formats to server-side formats. It properly handles:
- Time range calculations using relative time expressions
- Mapping of filter arrays while preserving operator and value pairs
- Setting appropriate default values for missing filters
- Structuring cursor information for pagination
This is a good example of the Single Responsibility Principle in action.
11-14:Details
❓ Verification inconclusive
Verify time conversion accuracy
The
getTimestampFromRelativefunction is used to convert relative time expressions to timestamps. Ensure this function correctly handles all expected relative time formats used in the application.
🏁 Script executed:
#!/bin/bash # Check the implementation of getTimestampFromRelative grep -A 20 "export function getTimestampFromRelative" apps/dashboard/lib/utils.tsLength of output: 83
I'll need to gather more details on the implementation of
getTimestampFromRelative, as the initial grep command produced no output. It seems that either the function isn’t defined in the expected file (apps/dashboard/lib/utils.ts) or it might exist elsewhere. Please manually verify the following:
- Location Check: Confirm that
getTimestampFromRelativeis defined somewhere in the codebase (e.g., using a repository-wide search likerg "getTimestampFromRelative" -n .).- Implementation Verification: Ensure that the implementation correctly parses all the relative time formats expected in our application.
Once verified, update or document the function as needed to avoid any potential time conversion issues.
Next Steps for the Developer:
- Search for the function: Run a grep (or similar) command to locate its definition.
- Confirm correctness: Double-check that the logic properly handles all anticipated relative time inputs.
apps/dashboard/app/(app)/audit/page.tsx (3)
5-6: Simplified import structureThe imports have been simplified by removing unused components and directly importing from the actions file.
11-13: Enhanced data retrievalThe component now destructures both workspace and members from the getWorkspace response, enabling access to organization members data which is likely needed for the logs client component.
22-37: Simplified component structureThe page structure has been simplified by:
- Removing the
PageContentandFilterscomponents- Directly using the
LogsClientcomponent with necessary props- Maintaining the empty state rendering
This refactoring improves code readability and centralizes more logic in the
LogsClientcomponent.apps/dashboard/app/(app)/audit/components/table/query-logs.schema.ts (3)
3-4: Defined constant for default bucketGood practice to define the default bucket name as a constant, making it easier to update if needed and providing clarity about its purpose.
4-47: Well-structured schema definitionThe Zod schema for audit query logs is comprehensive and well-structured:
- It defines required fields with appropriate types (limit, startTime, endTime, since)
- Sets default values where appropriate (bucket)
- Properly structures filter objects with operators and values
- Handles nullable fields appropriately
- Includes cursor information for pagination
This schema will provide both runtime validation and clear type information.
49-49: Type export for TypeScript integrationExporting the TypeScript type inferred from the Zod schema ensures type consistency across the application.
apps/dashboard/app/(app)/audit/components/controls/components/logs-datetime/index.tsx (1)
79-79:Details
❓ Verification inconclusive
Implement keyboard shortcut as mentioned in the title attribute
The button has a title attribute indicating "Press 'T' to toggle filters", but there's no implementation of this shortcut in the component. Based on past learnings, keyboard shortcuts should be implemented in the component that owns the state.
Could you confirm if the 'T' shortcut is implemented in the
DatetimePopovercomponent?
🏁 Script executed:
#!/bin/bash # Check if the DatetimePopover component implements the 'T' keyboard shortcut rg -A 10 -B 10 "function DatetimePopover" --include="*.tsx"Length of output: 503
Action Required: Verify 'T' Shortcut Implementation in DatetimePopover
- The title "Press 'T' to toggle filters" in this logs-datetime component suggests that the 'T' key should toggle filters.
- However, no keyboard event handling is implemented here.
- Based on our established guidelines, keyboard shortcuts must be managed in the stateful component (e.g.,
DatetimePopover), not in the presentational wrapper.- Please confirm if the 'T' shortcut is implemented in the
DatetimePopovercomponent.
- If it is, consider removing or revising the title here.
- If it isn’t, please implement the shortcut within
DatetimePopover.apps/dashboard/app/(app)/audit/components/table/hooks/use-logs-query.ts (1)
109-115:Details
❓ Verification inconclusive
Consider periodic refetching for better data freshness
The query configuration disables refetching on mount and window focus, which could affect data freshness. Consider enabling periodic refetching or providing a manual refresh option to ensure users see up-to-date data.
Is there a manual refresh option available elsewhere in the UI? If not, consider adding refetch capability or enabling periodic refetching.
🏁 Script executed:
#!/bin/bash # Check if there's a refresh component or button in the audit logs UI rg -A 5 -B 5 "LogsRefresh" --include="*.tsx"Length of output: 486
Audit Logs Refetch and Refresh Strategy
The current query configuration disables both mount and window-focus refetching, which may lead to stale audit log data. Our initial check for a manual refresh component (specifically a
LogsRefreshelement) encountered issues and did not provide conclusive results. Please verify directly whether a manual refresh button exists elsewhere in the UI. If not, consider one of the following improvements:
- Add a manual refresh control: Implement a dedicated refresh component to allow users to actively update the audit logs.
- Enable periodic refresh: Configure the query (or use an alternative strategy) to periodically refetch data for improved data freshness.
apps/dashboard/app/(app)/audit/actions.ts (2)
2-2: Import statement looks good.Bringing in
clerkClientfrom@clerk/nextjsappears correct for organization membership retrieval.
43-48: Exposing keys from the workspace query is properly structured.Selecting ID and name for the workspace keys is concise and consistent with the code’s style.
apps/dashboard/app/(app)/audit/components/logs-client.tsx (3)
3-4: Imports are well organized and minimal.The addition of the
AuditLogimport and related state hooks is clear and logical.
27-33: Cleanly encapsulated callback for layout adjustments.Using
useCallbackto updatetableDistanceToTopis both performant and readable.
45-45: Good alignment of thedistanceToTopprop with updated state.Ensuring the
AuditLogDetailscomponent references the same state variable for layout painting is a neat approach.apps/dashboard/lib/trpc/routers/audit/llm-search/utils.ts (5)
1-9: Importing schemas and types is correct.All dependencies look properly declared, supporting the LLM-based structured audit search logic.
10-18: Null return for missingopenaikeys might cause confusion.You return
nullifopenaiis not configured. Ensure callers gracefully handle this scenario, possibly gating LLM-based features or displaying an appropriate message to the user.
20-39: Usingbeta.chat.completions.parseis fine, but verify model availability.
model: "gpt-4o-mini"is presumably a specialized or hosted model. Double-check that the environment supports this model.
41-53: Graceful handling of unparsed responses.Throwing a
TRPCErrorwhen no parsed result ensures consistent fallback messaging for the user. The instructions for phrasing queries are helpful.
67-79: Robust error handling covers rate limits and unknown errors.Your approach ensures relevant, user-friendly TRPC errors are thrown, with fallback to
INTERNAL_SERVER_ERRORif no subtype matches.apps/dashboard/lib/trpc/routers/audit/schema.ts (3)
13-15: Clarify whethertimeproperty is seconds or milliseconds
Thetimeproperty is a number, but it's unclear if it represents a Unix timestamp in milliseconds or seconds. Adding explicit naming or documentation can help avoid confusion and future bugs.Also applies to: 37-37
39-67: Structured approach for joined data
Defining a separateAuditLogWithTargetstype is a clean solution for managing expanded log data. This approach looks maintainable.
69-101: Verify handling of nullable cursor fields
Fields likecursorTimeandcursorAuditIdare nullable. Ensure the query and pagination logic handle these values gracefully if they’re omitted or null.apps/dashboard/app/(app)/audit/components/table/utils/get-row-class.ts (2)
29-74: Well-structured style objects
Defining color-coded style objects for each event type is organized and enhances readability.
79-82: Handle undefined or empty events
getAuditStatusStyledefaults to "other" if no pattern is found. EnsureauditLog.eventis never an empty or undefined string, or handle that case explicitly.apps/dashboard/app/(app)/audit/filters.schema.ts (2)
51-55: Structured output schema usage
UsingcreateFilterOutputSchemais a succinct approach to maintain type safety and clarity in generating filter outputs. No issues found.
77-85: Confirm optional fields coverage
QuerySearchParamsincludes several nullable fields. Verify code that consumes them accounts for possiblenullor undefined values.apps/dashboard/lib/trpc/routers/audit/fetch.ts (2)
10-19: Clear and concise response schema
Defining theAuditLogsResponseZod schema ensures consumers receive a well-structured response. This is good for maintainability.
23-85: Consider edge cases for empty results
IfqueryAuditLogsreturns an empty set, ensure your UI and downstream usage handle it properly. The error handling looks good, but verify partial data scenarios.
.../app/(app)/audit/components/controls/components/logs-filters/components/root-keys-filter.tsx
Show resolved
Hide resolved
apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/index.tsx
Outdated
Show resolved
Hide resolved
...ard/app/(app)/audit/components/controls/components/logs-filters/components/bucket-filter.tsx
Show resolved
Hide resolved
apps/dashboard/app/(app)/audit/components/controls/components/logs-search/index.tsx
Show resolved
Hide resolved
apps/dashboard/app/(app)/audit/components/controls/components/logs-search/index.tsx
Show resolved
Hide resolved
apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/dashboard/app/(app)/audit/actions.ts (1)
50-57:⚠️ Potential issueAvoid loading ALL keys of a workspace unnecessarily.
This query fetches all keys for the workspace without any filtering or limit, which could be an expensive operation if there are many keys.
Consider one of these approaches:
- Add pagination or limit if only a subset of keys is needed
- Remove this query entirely if the keys aren't being used downstream
- Add filtering if only specific keys are needed
- const rootKeys = await db.query.keys.findMany({ - where: (table, { eq }) => eq(table.forWorkspaceId, workspace?.id), - - columns: { - id: true, - name: true, - }, - }); + // Only fetch keys if needed and consider adding limits + const rootKeys = []; + // If keys are needed, consider: + // const rootKeys = await db.query.keys.findMany({ + // where: (table, { eq }) => eq(table.forWorkspaceId, workspace?.id), + // columns: { + // id: true, + // name: true, + // }, + // limit: 100, // Add appropriate limit + // });
🧹 Nitpick comments (7)
apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/components/events-filter.tsx (3)
16-21: Consider using the event value as ID instead of array index.Using array indices for IDs could cause issues if the order of events changes in future updates. Using the event values themselves as unique identifiers would be more robust.
- options={Object.values(unkeyAuditLogEvents.Values).map<EventsOption>((value, index) => ({ - id: index, + options={Object.values(unkeyAuditLogEvents.Values).map<EventsOption>((value) => ({ + id: value, display: value, label: value, checked: false, }))}
5-10: Consider adding alphabetical sorting for better usability.The events are displayed in the order they appear in the schema, which might make it difficult for users to find specific events in a long list. Consider sorting them alphabetically.
options={Object.values(unkeyAuditLogEvents.Values) + .sort((a, b) => a.localeCompare(b)) .map<EventsOption>((value) => ({Also applies to: 16-21
24-26: Enhance accessibility for the option content.The rendered option content doesn't include any accessibility attributes. Consider adding appropriate ARIA attributes to improve accessibility.
renderOptionContent={(checkbox) => ( - <div className="text-accent-12 text-xs">{checkbox.display}</div> + <div + className="text-accent-12 text-xs" + aria-label={`Event type: ${checkbox.display}`} + > + {checkbox.display} + </div> )}apps/dashboard/app/(app)/logs/components/controls/components/logs-filters/components/methods-filter.tsx (1)
19-19: Consider deferring any major type-safety changes.
Per the retrieved learnings, further handler improvements may be needed once real data is integrated, so keep this flexible.apps/dashboard/components/logs/checkbox/filter-checkbox.tsx (3)
21-47: Helpful expansion of props for flexible filtering.
Defining single vs. multiple selection modes, optional default selection, and externalfilters/updateFilterscreates a comprehensive interface.Consider adding a quick doc comment explaining each prop’s intended usage for better discoverability.
82-186: Robust handling of single vs. multiple selection.
The combination ofhandleSingleSelection, default selection logic, andhandleApplyFiltercovers most use cases. A minor safeguard to check ifdefaultSelectionIndexexceedsoptions.lengthcould prevent out-of-bounds access.+ if (defaultSelectionIndex !== undefined && defaultSelectionIndex < checkboxes.length) { handleSingleSelection(defaultSelectionIndex); + }
227-228: Improve keyboard interaction for checkboxes.
As with the select-all label, using native<input type="checkbox" />references would automatically handle keyboard interactions, simplifying your event logic.Also applies to: 233-233
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/dashboard/app/(app)/audit/actions.ts(2 hunks)apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/components/bucket-filter.tsx(1 hunks)apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/components/events-filter.tsx(1 hunks)apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/components/root-keys-filter.tsx(1 hunks)apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/components/users-filter.tsx(1 hunks)apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/index.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/controls/components/logs-filters/components/methods-filter.tsx(3 hunks)apps/dashboard/app/(app)/logs/components/controls/components/logs-filters/components/status-filter.tsx(3 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-filters/components/status-filter.tsx(3 hunks)apps/dashboard/components/logs/checkbox/filter-checkbox.tsx(4 hunks)apps/dashboard/components/logs/refresh-button/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/components/bucket-filter.tsx
- apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/index.tsx
- apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/components/root-keys-filter.tsx
- apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/components/users-filter.tsx
🧰 Additional context used
🧠 Learnings (1)
apps/dashboard/app/(app)/logs/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/components/logs/checkbox/filter-checkbox.tsx
[error] 200-216: 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] 201-203: 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] 223-225: 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 (9)
- 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: Test Agent Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: autofix
- GitHub Check: Docs
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (15)
apps/dashboard/components/logs/refresh-button/index.tsx (1)
64-64: Improved keyboard shortcut representation.Changing from the symbol representation
"⌃"to the text-based"CTRL"makes the keyboard shortcut more accessible and platform-agnostic. This update aligns with similar changes in other components, ensuring consistent keyboard shortcut display across the application.apps/dashboard/app/(app)/audit/actions.ts (3)
2-2: Added new Clerk integration.Good addition of the Clerk client import to support fetching organization members.
7-30: Well-structured organization members retrieval.The implementation correctly:
- Only fetches members for organizational tenants (not user tenants)
- Filters out members without public user data
- Creates a structured format with appropriate labels
- Implements proper error handling
The error message is also descriptive and includes the tenant ID for easier debugging.
59-59: Enhanced return structure to support new filtering capabilities.The updated return structure now includes both workspace data (with keys) and organization members, which enables the audit log controls to filter by user.
apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/components/events-filter.tsx (2)
5-10: Good type definition for EventsOption.The type is well-structured and clearly defines the expected shape of each filter option with appropriate properties.
1-34: Well-structured component with good separation of concerns.The component follows good practices by leveraging the
useFiltershook for state management and theFilterCheckboxcomponent for rendering. This maintains a clean separation of concerns and makes the code more maintainable.apps/dashboard/app/(app)/logs/components/controls/components/logs-filters/components/methods-filter.tsx (2)
1-1: Looks good.
ImportinguseFiltershere ensures we can synchronize filter states seamlessly.
31-32: Great integration with theFilterCheckboxcomponent.
PassingfiltersandupdateFiltersaligns with the standard filter pattern used throughout the codebase.apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-filters/components/status-filter.tsx (3)
2-2: Neat import.
Bringing inuseFiltersfrom a central location promotes consistent filter management across the app.
30-30: Well-structured usage ofuseFilters.
Destructuring the filter state here keeps the component concise and maintainable.
45-46: Consistent props forFilterCheckbox.
UsingfiltersandupdateFiltersensures compatibility with other filter components.apps/dashboard/app/(app)/logs/components/controls/components/logs-filters/components/status-filter.tsx (3)
1-1: Nice addition of theuseFiltershook.
This approach standardizes the filtering logic across multiple components.
42-42: Efficient access to filter state.
ExtractingfiltersandupdateFilterslets you inject them directly into the UI without extra boilerplate.
66-67: Seamless integration withFilterCheckbox.
Passing alongfiltersandupdateFiltersensures that changes remain in sync with other filters.apps/dashboard/components/logs/checkbox/filter-checkbox.tsx (1)
70-80: Smooth integration withuseCheckboxState.
By embedding filter synchronization logic in the hook call, you keep this component’s logic simpler and more testable.
...ard/app/(app)/audit/components/controls/components/logs-filters/components/events-filter.tsx
Show resolved
Hide resolved
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
apps/dashboard/app/(app)/audit/components/table/logs-table.tsx (4)
28-31: Consider safeguarding against potential layout shiftsThe measurement of table position happens only once on mount. This could lead to inaccurate positioning if the layout shifts after initial render.
Consider adding a resize observer or re-measuring on window resize events:
useEffect(() => { const distanceToTop = tableRef.current?.containerRef?.getBoundingClientRect().top ?? 0; onMount(distanceToTop); + + const handleResize = () => { + const newDistanceToTop = tableRef.current?.containerRef?.getBoundingClientRect().top ?? 0; + onMount(newDistanceToTop); + }; + + window.addEventListener('resize', handleResize); + return () => window.removeEventListener('resize', handleResize); }, [onMount]);
79-97: Consider simplifying the conditional actor rendering logicThe current implementation uses nested conditional rendering with repeated structure, which could be simplified.
Consider using a more declarative approach:
render: (log) => { const actorType = log.auditLog.actor.type; let icon = <FunctionSquare className="w-4 h-4" />; let content = log.auditLog.actor.id; if (actorType === "user" && log.user) { icon = null; content = `${log.user.firstName ?? ""} ${log.user.lastName ?? ""}`; } else if (actorType === "key") { icon = <KeySquare className="w-4 h-4" />; } return ( <div className="flex items-center gap-3 truncate"> <div className="flex items-center w-full gap-2 max-sm:m-0 max-sm:gap-1 max-sm:text-xs"> {icon} <span className={cn("text-xs", actorType !== "user" && "font-mono truncate")}> {content} </span> </div> </div> ); },
128-131: Consider handling long event names more gracefullyThe current truncation of event names might cut off important information.
Consider adding a tooltip or hover state to show the full event name:
render: (log) => ( <div className="flex items-center gap-2 font-mono text-xs truncate"> - <span>{log.auditLog.event}</span> + <span title={log.auditLog.event}>{log.auditLog.event}</span> </div> ),
137-139: Same truncation issue applies to the description fieldThe description field is fixed at 200px width with truncation, which might hide important information.
Add a tooltip to show the full description:
render: (log) => ( - <div className="font-mono text-xs truncate w-[200px]">{log.auditLog.description}</div> + <div className="font-mono text-xs truncate w-[200px]" title={log.auditLog.description}> + {log.auditLog.description} + </div> ),apps/dashboard/app/(app)/audit/components/table/utils/get-row-class.ts (2)
7-27: Consider making event type classification more robustThe current string-based classification might be fragile if event naming conventions change. A more structured approach would be more maintainable.
Consider using a pattern matching approach or predefined event types:
+ type EventCategory = "create" | "update" | "delete" | "other"; + + // Map of keywords to event categories for more maintainable classification + const EVENT_CATEGORY_KEYWORDS: Record<EventCategory, string[]> = { + create: ["create", "add", "new"], + update: ["update", "edit", "modify", "change"], + delete: ["delete", "remove", "destroy"], + other: [] + }; export const getEventType = (event: string): "create" | "update" | "delete" | "other" => { const eventLower = event.toLowerCase(); + // Find the first category where any of its keywords match the event name + const category = Object.entries(EVENT_CATEGORY_KEYWORDS).find( + ([_, keywords]) => keywords.some(keyword => eventLower.includes(keyword)) + )?.[0] as EventCategory; + + return category || "other"; - if (eventLower.includes("create") || eventLower.includes("add")) { - return "create"; - } - - if ( - eventLower.includes("update") || - eventLower.includes("edit") || - eventLower.includes("modify") - ) { - return "update"; - } - - if (eventLower.includes("delete") || eventLower.includes("remove")) { - return "delete"; - } - - return "other"; };
108-115: Consider handling undefined or null event typesThe function doesn't handle cases where the event might be undefined or null.
Add a safety check:
export const getAuditSelectedClassName = (item: AuditLog, isSelected: boolean) => { if (!isSelected) { return ""; } - const style = AUDIT_STATUS_STYLES[getEventType(item.auditLog.event)]; + const eventType = item.auditLog.event ? getEventType(item.auditLog.event) : "other"; + const style = AUDIT_STATUS_STYLES[eventType]; return style.selected; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/dashboard/app/(app)/audit/components/table/log-details/components/log-header.tsx(1 hunks)apps/dashboard/app/(app)/audit/components/table/logs-table.tsx(1 hunks)apps/dashboard/app/(app)/audit/components/table/utils.ts(0 hunks)apps/dashboard/app/(app)/audit/components/table/utils/get-row-class.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/dashboard/app/(app)/audit/components/table/utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/app/(app)/audit/components/table/log-details/components/log-header.tsx
🧰 Additional context used
🧠 Learnings (1)
apps/dashboard/app/(app)/audit/components/table/logs-table.tsx (1)
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2143
File: apps/dashboard/app/(app)/logs/logs-page.tsx:77-83
Timestamp: 2024-12-03T14:17:08.016Z
Learning: The `<LogsTable />` component already implements virtualization to handle large datasets efficiently.
⏰ 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: Build / Build
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
apps/dashboard/app/(app)/audit/components/table/logs-table.tsx (1)
1-57: Virtual table implementation looks solid with good component structureThe AuditLogsTable component is well-structured with clean separation of concerns, proper TypeScript typing, and effective use of the virtual table pattern for performance. The distance calculation in the useEffect hook ensures proper positioning, and the props interface provides clear expectations for parent components.
apps/dashboard/app/(app)/audit/components/table/utils/get-row-class.ts (2)
29-74: This explains the green row UI comment in the PRThe styling configuration explains why rows might appear green - "create" events use a green color scheme. This addresses the PR comment about "why is the entire row green now?"
The color system is comprehensive and follows a consistent pattern across different event types. The use of semantic color variables (success, warning, etc.) is a good practice.
87-103:Details
❓ Verification inconclusive
Ensure accessibility with proper color contrast for selected states
The styling looks visually appealing but should be verified for proper accessibility contrast ratios, especially for selected states.
🌐 Web query:
What are the WCAG contrast guidelines for interactive UI elements?💡 Result:
WCAG guidelines require interactive UI elements like buttons, form controls, and indicators to maintain a minimum 3:1 contrast ratio against adjacent colors under Success Criterion 1.4.11 (Level AA) [1][3][7]. Here's a detailed breakdown:
Core Requirements for UI Components
- Visual boundaries: Borders/edges of input fields, buttons, or tabs must have ≥3:1 contrast against backgrounds ([Example of insufficient 1.5:1 borders in forms][1]).
- State indicators: Hover, focus, and active states (e.g., button pressed) require ≥3:1 contrast [3][7].
- Graphical controls: Icons in toolbars, toggle switches, and checkboxes need ≥3:1 against their backgrounds ([Example of failing 2.7:1 checkbox borders][9]).
Text Within UI Components
While UI elements themselves follow the 3:1 rule:
- Button labels/input text must meet standard text contrast:
- 4.5:1 for normal text (≤18pt regular/≤14pt bold)
- 3:1 for large text (>18pt regular/>14pt bold) [3][4][8].
Exceptions
- Disabled components: Inactive elements (e.g., grayed-out buttons) are exempt but should still be detectable if their presence conveys information [1][9].
- Decorative elements: Non-informative graphics/logos don’t require compliance [7][8].
Enhanced Guidelines (Level AAA)
For stricter compliance:
- Text contrast rises to 7:1 (normal) or 4.5:1 (large), but UI components remain at ≥3:1 [2][4][8].
Common Failures and Fixes
Issue Example Solution Low-contrast borders Input field borders at 1.5:1 ([Fig. 2][1]) Use darker borders (e.g., #767676 on white = 4.6:1) Subtle focus rings Gray (#AAA) focus indicator at 2.3:1 ([Fig. 10][9]) Add high-contrast outlines (e.g., black on white) Icon-only buttons Dropdown arrows at <3:1 ([Fig. 10][9]) Use solid shapes instead of outlines Designers should validate contrasts using tools like WebAIM’s Contrast Checker or Stark plugin [6][8].
Citations:
- 1: https://www.accessibility-developer-guide.com/knowledge/colours-and-contrast/user-interface-components/
- 2: https://www.oregon.gov/ode/accessibility/checklist/pages/contrast.aspx
- 3: https://accessibleweb.com/question-answer/are-there-different-contrast-ratio-requirements-for-text-and-for-ui-elements/
- 4: https://www.getstark.co/wcag-explained/perceivable/distinguishable/contrast-minimum/
- 5: https://www.maine.edu/content-management/accessibility/color-ratio/
- 6: https://pimpmytype.com/color-contrast/
- 7: https://daily.dev/blog/color-contrast-guidelines-for-text-and-ui-accessibility
- 8: https://www.boia.org/blog/designing-for-color-contrast-guidelines-for-accessibility
- 9: https://uxpamagazine.org/color-contrast-infographics-and-ui-accessibility/
- 10: https://www.w3.org/WAI/WCAG21/Understanding/non-text-contrast.html
Ensure proper WCAG compliance for selected state styling
The function currently conditionally applies styling via
style.selectedand adjusts opacity based on the state. Please verify that these styles (including any focus ring or hover effects) meet the WCAG minimum contrast ratio of 3:1 for interactive UI elements (per WCAG 1.4.11 Level AA). We recommend using a tool like WebAIM’s Contrast Checker to confirm that both the selected and unselected states, as well as any focus indicators, provide sufficient contrast.
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
Refactor
VirtualTablecomponent to a forward-ref component for better ref handling.Bug Fixes