refactor: improved audit table #2757
Conversation
ogzhanolguncu
requested review from
perkinsjr,
chronark,
mcstepp and
MichaelUnkey
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces a comprehensive enhancement to the audit log functionality in the dashboard application. The changes include creating new components for filtering, displaying, and managing audit logs, implementing a virtual table for log rendering, and adding utility functions for event type classification and parameter parsing. The modifications span multiple files across the dashboard, focusing on improving the user interface, data fetching, and interaction with audit log data. Changes
Suggested labels
Suggested reviewers
Possibly related PRs
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: 11
🔭 Outside diff range comments (1)
apps/dashboard/app/(app)/audit/[bucket]/filter.tsx (1)
Line range hint
85-91: Simplify event handling to avoid potential conflictsThe component has overlapping click and keyboard event handlers. Consider consolidating them into a single handler.
- onClick={() => handleSelection(option.value, isSelected)} - onKeyDown={(e) => { - if (e.key === "Enter" || e.key === " ") { - e.preventDefault(); - handleSelection(option.value, isSelected); - } - }} + onClick={(e) => { + if (e.type === 'click' || (e.type === 'keydown' && (e.key === 'Enter' || e.key === ' '))) { + e.preventDefault(); + handleSelection(option.value, isSelected); + } + }}
🧹 Nitpick comments (19)
internal/icons/src/icons/clone-x-mark-2.tsx (1)
13-15: Optimize React import for better tree-shakingThe static analyzer correctly identifies that React is only used for types.
-import React from "react"; +import type React from "react"; import type { IconProps } from "../props";🧰 Tools
🪛 Biome (1.9.4)
[error] 13-13: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.(lint/style/useImportType)
internal/icons/src/icons/clone-plus-2.tsx (1)
26-39: Consider simplifying the rectangle transformationThe rectangle uses a transform to rotate 180 degrees, which could be simplified by directly setting the coordinates.
<rect height="11" width="11" fill="none" rx="2" ry="2" stroke="currentColor" strokeLinecap="round" strokeLinejoin="round" strokeWidth="1.5" - transform="translate(21.5 21.5) rotate(180)" x="5.25" y="5.25" />apps/dashboard/app/(app)/audit/[bucket]/components/table/types.ts (2)
26-26: Consider stronger typing for themetafield.The
unknowntype formetais too permissive and could lead to runtime type errors. Consider defining a more specific type based on the expected structure of the metadata.
1-30: Add JSDoc documentation for the Data type.This type is used across multiple components. Adding comprehensive documentation would help other developers understand the purpose and structure of each field.
Example documentation:
/** * Represents an audit log entry with associated user information. * @property user - Optional user information associated with the audit log * @property auditLog - Detailed information about the audit event * @property auditLog.targets - Array of affected resources/entities */ export type Data = { // ... rest of the type definition };apps/dashboard/app/(app)/audit/[bucket]/components/log-header.tsx (2)
20-22: Improve button accessibility.The close button lacks proper accessibility attributes.
Add aria-label to make the button's purpose clear to screen readers:
-<Button shape="square" variant="ghost" onClick={onClose}> +<Button + shape="square" + variant="ghost" + onClick={onClose} + aria-label="Close audit log details" +> <X size="22" strokeWidth="1.5" className="text-content/65 cursor-pointer" /> </Button>
11-26: Consider wrapping component in error boundary.The component directly accesses nested properties (
log.auditLog.event) without any error handling. Consider adding an error boundary to gracefully handle potential runtime errors.Example implementation:
import { ErrorBoundary } from '@/components/error-boundary'; export const LogHeader = ({ onClose, log }: Props) => { return ( <ErrorBoundary fallback={<div>Error loading audit log header</div>}> {/* existing component code */} </ErrorBoundary> ); };apps/dashboard/app/(app)/audit/[bucket]/query-state.ts (2)
3-16: Add JSDoc comments to improve type documentation.The
CursorandAuditLogQueryParamstypes would benefit from JSDoc comments explaining their purpose and the significance of each field.Example improvement:
+/** + * Represents a pagination cursor for audit log entries + * @property time - Timestamp of the current position + * @property id - Unique identifier at the current position + */ export type Cursor = { time: number; id: string; }; +/** + * Query parameters for filtering and paginating audit logs + * @property before - Timestamp to filter entries before + * @property events - Array of event types to filter + * @property users - Array of user IDs to filter + * @property rootKeys - Array of root keys to filter + * @property bucket - Bucket identifier + * @property cursorTime - Cursor timestamp for pagination + * @property cursorId - Cursor ID for pagination + */ export type AuditLogQueryParams = {
28-43: Add error handling for invalid cursor values.The
setCursorfunction should validate the cursor values before setting them.Consider this improvement:
const setCursor = (cursor?: Cursor) => { + if (cursor && (isNaN(cursor.time) || !cursor.id)) { + console.warn('Invalid cursor values provided:', cursor); + return; + } setSearchParams({ cursorTime: cursor?.time ?? null, cursorId: cursor?.id ?? null, }); };apps/dashboard/app/(app)/audit/[bucket]/components/table/constants.ts (1)
3-59: Enhance type safety and maintainability of event groups.While the event grouping is comprehensive, it could benefit from:
- Type safety using string literals
- Centralized event name constants
- Helper functions for type checking
Consider this improvement:
// Define event names as string literals for type safety export const EventNames = { WORKSPACE: { CREATE: 'workspace.create', DELETE: 'workspace.delete', UPDATE: 'workspace.update', OPT_IN: 'workspace.opt_in' }, // ... similar for other categories } as const; // Type-safe event groups export const eventGroups = { create: [ EventNames.WORKSPACE.CREATE, // ... other create events ], // ... other groups } as const; // Helper function for type checking export const isValidEventType = (event: string): event is typeof eventGroups[keyof typeof eventGroups][number] => { return Object.values(eventGroups).some(group => group.includes(event as any)); };apps/dashboard/app/(app)/audit/[bucket]/components/table/table-details.tsx (1)
20-26: Consider using ResizeObserver instead of debounce.The debounced width update might cause jittery resizing. ResizeObserver would provide smoother updates.
-const [panelWidth, setPanelWidth] = useState(DEFAULT_DRAGGABLE_WIDTH); -const debouncedSetPanelWidth = useDebounceCallback((newWidth) => { - setPanelWidth(newWidth); -}, PANEL_WIDTH_SET_DELAY); +const panelRef = useRef<HTMLDivElement>(null); +const [panelWidth, setPanelWidth] = useState(DEFAULT_DRAGGABLE_WIDTH); + +useEffect(() => { + if (!panelRef.current) return; + const observer = new ResizeObserver((entries) => { + const width = entries[0]?.contentRect.width; + if (width) setPanelWidth(width); + }); + observer.observe(panelRef.current); + return () => observer.disconnect(); +}, []);apps/dashboard/app/(app)/audit/[bucket]/components/log-section.tsx (2)
10-11: Consider adding type validation for the details propThe
detailsprop accepts either a string or string array, but there's no validation to ensure non-empty values.Consider adding runtime validation or TypeScript constraints:
- details: string | string[]; + details: NonNullable<string | string[]>;
61-72: Optimize getFormattedContent functionThe function could be simplified and made more robust.
const getFormattedContent = (details: string | string[]) => { if (Array.isArray(details)) { - return details + return details.filter(Boolean) .map((header) => { const [key, ...valueParts] = header.split(":"); const value = valueParts.join(":").trim(); return `${key}: ${value}`; }) .join("\n"); } - return details; + return details?.trim() || ""; };apps/dashboard/app/(app)/audit/[bucket]/components/table/columns.tsx (2)
52-59: Consider extracting badge styling logicThe badge styling logic could be moved to a separate utility function for better maintainability.
+ const getBadgeClassName = (eventType: string) => + cn("font-mono capitalize", { + "bg-error-3 text-error-11 hover:bg-error-4": eventType === "delete", + "bg-warning-3 text-warning-11 hover:bg-warning-4": eventType === "update", + "bg-success-3 text-success-11 hover:bg-success-4": eventType === "create", + "bg-accent-3 text-accent-11 hover:bg-accent-4": eventType === "other", + }); render: (log) => { const eventType = getEventType(log.auditLog.event); - const badgeClassName = cn("font-mono capitalize", { - "bg-error-3 text-error-11 hover:bg-error-4": eventType === "delete", - "bg-warning-3 text-warning-11 hover:bg-warning-4": eventType === "update", - "bg-success-3 text-success-11 hover:bg-success-4": eventType === "create", - "bg-accent-3 text-accent-11 hover:bg-accent-4": eventType === "other", - }); + const badgeClassName = getBadgeClassName(eventType); return <Badge className={badgeClassName}>{eventType}</Badge>; },
1-8: Consider adding column sorting capabilityThe table could benefit from sorting functionality, especially for time-based entries.
Consider implementing sort handlers in the Column type and adding sort indicators to column headers. Would you like me to provide an example implementation?
apps/dashboard/app/(app)/audit/[bucket]/components/log-footer.tsx (2)
19-20: Memoize date formatting to prevent unnecessary recalculationsThe date formatting operation is performed on every render. Consider memoizing this value.
+ import { useMemo } from "react"; // ... - content: format(log.auditLog.time, "MMM dd HH:mm:ss.SS"), + content: useMemo(() => format(log.auditLog.time, "MMM dd HH:mm:ss.SS"), [log.auditLog.time]),
16-94: Extract field configurations for better maintainabilityThe fields array is quite large and could be extracted into a separate configuration file or constant.
Consider moving the fields configuration to a separate file like
log-footer.config.ts:// log-footer.config.ts export const getLogFields = (log: Data) => [ { label: "Time", description: (content: string) => <span className="text-[13px] font-mono">{content}</span>, content: format(log.auditLog.time, "MMM dd HH:mm:ss.SS"), tooltipContent: "Copy Time", tooltipSuccessMessage: "Time copied to clipboard", }, // ... other fields ];apps/dashboard/app/(app)/audit/[bucket]/filter.tsx (1)
46-73: Consider accessibility improvements for the filter triggerThe filter trigger button should have proper ARIA attributes for better screen reader support.
<Button className="flex items-center h-8 gap-2 " + aria-label={`${title} filter${selected.length > 0 ? `: ${selected.length} selected` : ''}`} + role="combobox" + aria-expanded={open} >apps/dashboard/app/(app)/audit/[bucket]/page.tsx (2)
Line range hint
76-93: Consider extracting the complex query logicThe audit log query is quite complex with multiple conditions. Consider extracting it into a separate database utility function for better maintainability and reusability.
Example refactor:
// db/queries/audit-logs.ts export async function getAuditLogs({ workspaceId, bucketName, selectedEvents, selectedActorIds, retentionCutoff, limit }: AuditLogQueryParams) { return db.query.auditLogBucket.findFirst({ where: (table, { eq, and }) => and(eq(table.workspaceId, workspaceId), eq(table.name, bucketName)), with: { logs: { where: (table, { and, inArray, gte }) => and( selectedEvents.length > 0 ? inArray(table.event, selectedEvents) : undefined, gte(table.createdAt, retentionCutoff), selectedActorIds.length > 0 ? inArray(table.actorId, selectedActorIds) : undefined, ), with: { targets: true, }, orderBy: (table, { desc }) => desc(table.time), limit, }, }, }); }
Line range hint
239-285: Consider adding error boundaries for filter componentsWhile the components handle null checks well, they could benefit from error boundaries to gracefully handle potential failures in data fetching or processing.
Example implementation:
class FilterErrorBoundary extends React.Component<{ children: React.ReactNode }> { state = { hasError: false }; static getDerivedStateFromError() { return { hasError: true }; } render() { if (this.state.hasError) { return <Filter param="" title="Error" options={[]} />; } return this.props.children; } } // Usage: <FilterErrorBoundary> <UserFilter tenantId={tenantId} /> </FilterErrorBoundary>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
apps/dashboard/app/(app)/audit/[bucket]/components/log-footer.tsx(1 hunks)apps/dashboard/app/(app)/audit/[bucket]/components/log-header.tsx(1 hunks)apps/dashboard/app/(app)/audit/[bucket]/components/log-section.tsx(1 hunks)apps/dashboard/app/(app)/audit/[bucket]/components/table/columns.tsx(1 hunks)apps/dashboard/app/(app)/audit/[bucket]/components/table/constants.ts(1 hunks)apps/dashboard/app/(app)/audit/[bucket]/components/table/index.tsx(1 hunks)apps/dashboard/app/(app)/audit/[bucket]/components/table/table-details.tsx(1 hunks)apps/dashboard/app/(app)/audit/[bucket]/components/table/types.ts(1 hunks)apps/dashboard/app/(app)/audit/[bucket]/components/table/utils.ts(1 hunks)apps/dashboard/app/(app)/audit/[bucket]/filter.tsx(2 hunks)apps/dashboard/app/(app)/audit/[bucket]/page.tsx(8 hunks)apps/dashboard/app/(app)/audit/[bucket]/query-state.ts(1 hunks)apps/dashboard/app/(app)/audit/[bucket]/row.tsx(0 hunks)apps/dashboard/app/(app)/logs/components/table/log-details/components/request-response-details.tsx(1 hunks)apps/dashboard/components/virtual-table.tsx(7 hunks)apps/dashboard/lib/trpc/routers/audit/fetch.ts(1 hunks)apps/dashboard/lib/trpc/routers/index.ts(2 hunks)apps/dashboard/lib/utils.ts(1 hunks)internal/db/src/schema/audit_logs.ts(4 hunks)internal/icons/src/icons/clone-plus-2.tsx(1 hunks)internal/icons/src/icons/clone-x-mark-2.tsx(1 hunks)internal/icons/src/icons/code-action.tsx(1 hunks)internal/icons/src/icons/folder-feather.tsx(1 hunks)internal/icons/src/index.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/dashboard/app/(app)/audit/[bucket]/row.tsx
✅ Files skipped from review due to trivial changes (1)
- internal/db/src/schema/audit_logs.ts
🧰 Additional context used
🪛 Biome (1.9.4)
internal/icons/src/icons/code-action.tsx
[error] 13-13: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.
(lint/style/useImportType)
internal/icons/src/icons/clone-plus-2.tsx
[error] 13-13: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.
(lint/style/useImportType)
internal/icons/src/icons/folder-feather.tsx
[error] 13-13: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.
(lint/style/useImportType)
internal/icons/src/icons/clone-x-mark-2.tsx
[error] 13-13: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.
(lint/style/useImportType)
🔇 Additional comments (17)
internal/icons/src/index.ts (1)
20-23: LGTM! New icon exports follow consistent pattern
The new exports maintain the established pattern and alphabetical ordering.
internal/icons/src/icons/code-action.tsx (3)
16-60: LGTM! Well-structured SVG icon implementation
The component:
- Properly spreads props for customization
- Uses semantic SVG elements
- Maintains consistent styling with currentColor
- Has appropriate viewBox and dimensions
13-15: 🛠️ Refactor suggestion
Use import type for better tree-shaking
The React import is used for JSX transformation, but IconProps is only used as a type.
-import React from "react";
+import React from "react";
+import type { IconProps } from "../props";
-import type { IconProps } from "../props";Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 13-13: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.
(lint/style/useImportType)
1-11: Verify license compliance
The component includes Nucleo's license which restricts redistribution and usage. Ensure compliance with these terms:
- No redistribution of icons
- Icons restricted to bundled product use only
✅ Verification successful
I'll verify if all icon-related files consistently include the Nucleo license and if there are any icon files that might be missing it.
License compliance verified for Nucleo icons
The license compliance check shows proper implementation:
- All icons under
internal/icons/src/icons/consistently include the Nucleo license header - The LICENSE file is present in the
internal/icons/directory - Other icon-related files outside the
internal/icons/directory are separate components and don't contain Nucleo assets
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other files use the same license header
rg -l "Copyright © Nucleo" | wc -l
Length of output: 39
Script:
#!/bin/bash
# Let's check which files contain the Nucleo license to ensure consistent usage
rg -l "Copyright © Nucleo"
# Also check if there are any other icon files without this license
fd -e tsx -e ts "icon|Icon" | grep -v node_modules
Length of output: 1241
internal/icons/src/icons/folder-feather.tsx (2)
13-15: Use import type for better tree-shaking
Same improvement as in code-action.tsx for better tree-shaking.
🧰 Tools
🪛 Biome (1.9.4)
[error] 13-13: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.
(lint/style/useImportType)
16-53: LGTM! Well-structured complex SVG implementation
The folder icon implementation:
- Follows consistent component structure
- Uses appropriate SVG path elements for complex shapes
- Maintains proper styling and scaling
internal/icons/src/icons/clone-x-mark-2.tsx (2)
16-72: SVG implementation looks good!
The implementation follows best practices:
- Uses currentColor for theming compatibility
- Consistent stroke widths (1.5)
- Proper viewBox and dimensions
- Well-structured paths with appropriate attributes
1-11: Verify compliance with Nucleo license terms
The icon is under Nucleo's license which restricts redistribution and usage. Please ensure:
- The project has a valid Nucleo license
- Usage complies with the bundling restrictions
✅ Verification successful
License compliance appears to be in order
The codebase shows proper implementation of Nucleo icons:
- There's a dedicated LICENSE file in the internal/icons directory containing Nucleo terms
- The engineering documentation explicitly mentions using "Nucleo UI Essential" collection
- All icon files consistently maintain the required copyright notices
- Icons are properly bundled within the product as part of the internal/icons package
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if Nucleo license exists in the project
fd -g "*license*" -i | grep -i nucleo
Length of output: 39
Script:
#!/bin/bash
# Let's check for any Nucleo-related files or references in the codebase
rg -i "nucleo" -A 3
# Also check package.json for any Nucleo dependencies
fd package.json --exec cat {} | grep -i "nucleo"
Length of output: 18912
internal/icons/src/icons/clone-plus-2.tsx (2)
13-15: Optimize React import for better tree-shaking
The static analyzer correctly identifies that React is only used for types.
-import React from "react";
+import type React from "react";
import type { IconProps } from "../props";🧰 Tools
🪛 Biome (1.9.4)
[error] 13-13: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.
(lint/style/useImportType)
16-73: SVG implementation looks good!
The implementation follows the same best practices as clone-x-mark-2:
- Uses currentColor for theming compatibility
- Consistent stroke widths (1.5)
- Proper viewBox and dimensions
- Well-structured paths with appropriate attributes
apps/dashboard/lib/trpc/routers/audit/fetch.ts (1)
1-97: Efficient Implementation of fetchAuditLog Procedure
The fetchAuditLog procedure is well-implemented with proper input validation, error handling, and pagination logic. The use of rate limiting and careful construction of database queries ensures both performance and security.
apps/dashboard/lib/utils.ts (1)
26-130: Well-Structured Throttle Utility Function
The throttle function is effectively implemented, providing configurable leading and trailing options, along with proper state management and helper methods. The detailed comments and type definitions enhance readability and maintainability.
apps/dashboard/app/(app)/logs/components/table/log-details/components/request-response-details.tsx (1)
70-70: LGTM: Style improvements enhance readability
The border and padding changes improve the visual hierarchy and spacing.
Also applies to: 75-75
apps/dashboard/app/(app)/audit/[bucket]/components/log-footer.tsx (1)
65-65: Review truncated text accessibility
The truncated text in User Agent and Description fields might affect keyboard navigation and screen reader accessibility. Consider adding a title attribute or aria-label.
Also applies to: 81-81
apps/dashboard/lib/trpc/routers/index.ts (2)
122-124: LGTM: Clean router syntax update
The simplified syntax for logs router methods is cleaner and consistent with modern JavaScript practices.
125-126: Verify fetchAuditLog implementation and error handling
The new audit router looks good, but let's ensure proper error handling in the fetchAuditLog implementation.
✅ Verification successful
Error handling is properly implemented in fetchAuditLog
The implementation includes comprehensive error handling:
- Database errors are caught and wrapped with appropriate TRPC errors
- Proper validation of workspace existence
- Clear error messages with support contact information
- Input validation using Zod schema
- Proper cursor-based pagination with limit checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check fetchAuditLog implementation for error handling
ast-grep --pattern 'export const fetchAuditLog = $_ => {
try {
$$$
} catch $_
}'
Length of output: 90
Script:
#!/bin/bash
# Let's first locate the fetchAuditLog implementation
rg "fetchAuditLog" -A 10
Length of output: 2358
Script:
#!/bin/bash
# Let's check the complete implementation of fetchAuditLog
cat apps/dashboard/lib/trpc/routers/audit/fetch.ts
Length of output: 3336
apps/dashboard/app/(app)/audit/[bucket]/page.tsx (1)
37-39: LGTM: Well-structured type definition
The AuditLogWithTargets type effectively combines the audit log with its targets, providing good type safety for the audit log data structure.
apps/dashboard/app/(app)/audit/[bucket]/components/table/index.tsx
Outdated
Show resolved
Hide resolved
apps/dashboard/app/(app)/audit/[bucket]/components/table/table-details.tsx
Outdated
Show resolved
Hide resolved
apps/dashboard/app/(app)/audit/[bucket]/components/table/table-details.tsx
Outdated
Show resolved
Hide resolved
apps/dashboard/app/(app)/audit/[bucket]/components/log-section.tsx
Outdated
Show resolved
Hide resolved
apps/dashboard/app/(app)/audit/[bucket]/components/log-footer.tsx
Outdated
Show resolved
Hide resolved
|
From Oguzhan Olguncu ‣ Instead of another PR, I added them back into this one. Both Andreas's comment and your datePicker issue is here |
|
Awesome let me test it again and we can go from there. |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
apps/dashboard/components/timestamp-info.tsx (1)
Line range hint
73-90: Improve accessibility and maintainability of the copy functionalityThe current implementation has several areas for improvement:
- Missing keyboard accessibility for copy functionality
- Hardcoded timeout value
- Missing ARIA labels for screen readers
Consider these improvements:
+const COPY_FEEDBACK_TIMEOUT = 1000; + return ( - //biome-ignore lint/a11y/useKeyWithClickEvents: no need <span + role="button" + tabIndex={0} + aria-label={`Copy ${label} value: ${value}`} onClick={(e) => { e.stopPropagation(); navigator.clipboard.writeText(value); setCopied(true); setTimeout(() => { setCopied(false); - }, 1000); + }, COPY_FEEDBACK_TIMEOUT); }} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + e.currentTarget.click(); + } + }} className={cn( "flex items-center hover:bg-background-subtle text-left px-3 py-2", {
🧹 Nitpick comments (7)
apps/dashboard/components/virtual-table.tsx (4)
71-97: Consider memoizing the calculateHeight functionWhile the height calculation logic is well-implemented, the
calculateHeightfunction is recreated on every render.Consider using
useCallback:+ const calculateHeight = useCallback(() => { - const calculateHeight = () => { if (!containerRef.current) { return; } const rect = containerRef.current.getBoundingClientRect(); const headerHeight = HEADER_HEIGHT + TABLE_BORDER_THICKNESS; const availableHeight = window.innerHeight - rect.top - headerHeight; setFixedHeight(Math.max(availableHeight, 0)); - }; + }, []);
99-111: Improve documentation for dependency exclusionThe
biome-ignorecomment could be more descriptive about why certain dependencies are excluded.Update the comment to be more informative:
- // biome-ignore lint/correctness/useExhaustiveDependencies: No need to add more deps + // biome-ignore lint/correctness/useExhaustiveDependencies: throttle wrapper is stable and doesn't need deps
135-144: Extract scroll threshold logic for better maintainabilityThe scroll threshold calculation logic could be more readable and maintainable.
Consider extracting the logic into a separate function:
+ const isNearBottom = (scrollElement: HTMLElement, threshold: number) => { + const scrollOffset = scrollElement.scrollTop + scrollElement.clientHeight; + return scrollOffset >= scrollElement.scrollHeight - threshold; + }; if ( !isLoading && !isFetchingNextPage && lastItem.index >= data.length - 1 - instance.options.overscan && - scrollOffset >= scrollThreshold + isNearBottom(scrollElement, rowHeight * 3) ) { throttledLoadMore(); }
334-341: Enhance loading indicator accessibilityThe loading indicator should have proper ARIA attributes for screen readers.
Add ARIA attributes to the loading indicator:
{isFetchingNextPage && ( <div className="fixed bottom-0 left-0 right-0 py-2 bg-background/90 backdrop-blur-sm border-t border-border" + role="status" + aria-live="polite" > <div className="flex items-center justify-center gap-2 text-sm text-accent-11"> <div className="h-1.5 w-1.5 rounded-full bg-accent-11 animate-pulse" + aria-hidden="true" /> Loading more data </div> </div> )}apps/dashboard/app/(app)/logs/components/filters/components/time-split.tsx (3)
Line range hint
37-143: Consider refactoring time validation logic for better maintainabilityThe time conflict resolution logic is complex and could benefit from refactoring:
- Extract date comparison into a helper function
- Simplify nested conditions using early returns
- Create separate functions for start/end time validations
Consider this approach:
const isSameDay = (startDate: Date, endDate: Date) => format(new Date(startDate), "dd/mm/yyyy") === format(new Date(endDate), "dd/mm/yyyy"); const validateStartTime = (startTime: Time, endTime: Time): Time | null => { if (Number(startTime.HH) > Number(endTime.HH)) { return { ...startTime }; } if (Number(startTime.HH) === Number(endTime.HH) && Number(startTime.mm) > Number(endTime.mm)) { return { ...startTime, ss: endTime.ss }; } // ... similar for seconds return null; };
Line range hint
145-182: Enhance input validation robustnessThe current validation could be improved to handle non-numeric inputs and simplified using a validation map.
Consider this enhancement:
const TIME_CONSTRAINTS = { HH: { max: 23 }, mm: { max: 59 }, ss: { max: 59 } } as const; function handleOnChange(value: string, valueType: TimeType) { if (!/^\d*$/.test(value) || value.length > 2) return; const numValue = Number(value); if (value && numValue > TIME_CONSTRAINTS[valueType].max) return; setTime(prev => ({ ...prev, [valueType]: value })); }
Line range hint
184-279: Enhance UI accessibility and maintainabilityWhile the UI implementation is solid, consider these improvements:
- The pattern attribute could be more specific for better validation
- Add role="group" and aria-labelledby for better screen reader support
- Consider extracting common input styles to a shared class
Example improvements:
- pattern="[0-23]*" + pattern="([0-1][0-9])|(2[0-3])" + inputMode="numeric" - <div className={`flex h-7 items-center justify-center w-fit px-4...`}> + <div + role="group" + aria-labelledby="time-input-label" + className={`flex h-7 items-center justify-center w-fit px-4...`} + > + <span id="time-input-label" className="sr-only">Time input</span>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/dashboard/app/(app)/logs/components/filters/components/time-split.tsx(1 hunks)apps/dashboard/components/timestamp-info.tsx(4 hunks)apps/dashboard/components/virtual-table.tsx(8 hunks)
🔇 Additional comments (4)
apps/dashboard/components/virtual-table.tsx (1)
2-2: LGTM: Type definitions and imports are well-structured
The addition of isFetchingNextPage to VirtualTableProps and the throttle utility import enhance the component's functionality for handling paginated data loading.
Also applies to: 30-30
apps/dashboard/components/timestamp-info.tsx (2)
6-7: LGTM: Import changes align with the new timestamp formatting approach
The replacement of formatRelative with ms library is appropriate for the new relative time implementation.
Line range hint 98-107: Address hover state concerns mentioned in PR comments
Based on the PR comments, there are unresolved concerns about the timestamp hover state from Vercel. Consider reviewing the tooltip trigger hover behavior and ensuring it works consistently across different screen sizes.
Let's check for any related hover state issues:
apps/dashboard/app/(app)/logs/components/filters/components/time-split.tsx (1)
5-5: Verify the removal of useEffect's impact on time synchronization
The removal of the useEffect hook means that time values won't automatically update when dates change. This could lead to invalid time states if not properly handled elsewhere in the parent components.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
apps/dashboard/components/virtual-table.tsx (1)
Line range hint
217-230: Enhance accessibility with proper ARIA attributesWhile keyboard navigation is implemented, the table could benefit from additional accessibility attributes:
<div className="w-full flex flex-col" ref={containerRef}> - <TableHeader /> + <div role="table" aria-label="Virtual Table"> + <div role="rowgroup"> + <TableHeader /> + </div> <div ref={(el) => { if (el) { //@ts-expect-error safe to bypass parentRef.current = el; //@ts-expect-error safe to bypass tableRef.current = el; } }} data-table-container="true" + role="rowgroup" className="overflow-auto pb-10" style={{ height: `${fixedHeight}px` }} >
🧹 Nitpick comments (7)
apps/dashboard/app/(app)/logs/components/filters/components/custom-date-filter.tsx (2)
30-33: Consider handling zero timestamp edge case.The current initialization using nullish coalescing (
??) with 0 as default might mask legitimate timestamp values of 0. Consider using a more explicit check:- startTime: parseAsInteger.withDefault(initialParams.startTime ?? 0), - endTime: parseAsInteger.withDefault(initialParams.endTime ?? 0), + startTime: parseAsInteger.withDefault( + initialParams.startTime === null ? 0 : initialParams.startTime + ), + endTime: parseAsInteger.withDefault( + initialParams.endTime === null ? 0 : initialParams.endTime + ),
Line range hint
20-156: Consider enhancing accessibility for the date picker.Since this component is part of an audit table, consider adding the following accessibility improvements:
- Add aria-label to the date picker trigger
- Include screen reader text for the time inputs
- Add aria-live region for announcing selected date ranges
Example improvements:
<PopoverTrigger asChild> <div id="date" + aria-label="Select date range" className={cn( "justify-start text-left font-normal flex gap-2 items-center", !finalDate && "text-muted-foreground", )} >apps/dashboard/app/(app)/audit/[bucket]/components/table/log-footer.tsx (2)
35-64: Consider extracting Actor Details into a separate component.The Actor Details field contains complex conditional rendering logic that would be better managed in a dedicated component.
+ // ActorDetails.tsx + type ActorDetailsProps = { + log: typeof log.auditLog; + user: typeof log.user; + }; + + const ActorDetails = ({ log, user }: ActorDetailsProps) => { + if (log.actor.type === "user" && user?.imageUrl) { + return ( + <div className="flex justify-end items-center w-full gap-2 max-sm:m-0 max-sm:gap-1 max-sm:text-xs md:flex-grow"> + <Avatar className="w-6 h-6"> + <AvatarImage src={user.imageUrl} /> + <AvatarFallback>{user?.username?.slice(0, 2)}</AvatarFallback> + </Avatar> + <span className="text-sm text-content whitespace-nowrap"> + {`${user?.firstName ?? ""} ${user?.lastName ?? ""}`} + </span> + </div> + ); + } + + const Icon = log.actor.type === "key" ? KeySquare : FunctionSquare; + return ( + <div className="flex items-center w-full gap-2 max-sm:m-0 max-sm:gap-1 max-sm:text-xs md:flex-grow"> + <Icon className="w-4 h-4" /> + <span className="font-mono text-xs text-content">{log.actor.id}</span> + </div> + ); + }; // In LogFooter.tsx { label: "Actor Details", description: (content) => <ActorDetails log={content.log} user={content.user} />, content: { log: log.auditLog, user: log.user }, className: "whitespace-pre", tooltipContent: "Copy Actor", tooltipSuccessMessage: "Actor copied to clipboard", },
18-26: Document the skipTooltip flag usage.The Time field has
skipTooltip: truebut it's not clear why this is needed when other fields have tooltips.Consider adding a comment explaining why the Time field skips the tooltip functionality.
apps/dashboard/components/virtual-table.tsx (3)
38-38: Document the rationale for THROTTLE_DELAY valueConsider adding a comment explaining why 350ms was chosen as the throttle delay. This helps future maintainers understand if this value needs adjustment based on specific performance requirements.
138-140: Extract magic number into a named constantThe calculation uses
rowHeight * 3as a threshold, but the '3' is a magic number. Consider extracting this into a named constant for better maintainability.+ const SCROLL_THRESHOLD_ROWS = 3; const scrollOffset = scrollElement.scrollTop + scrollElement.clientHeight; - const scrollThreshold = scrollElement.scrollHeight - rowHeight * 3; + const scrollThreshold = scrollElement.scrollHeight - rowHeight * SCROLL_THRESHOLD_ROWS;
323-330: Consider relative positioning for loading indicatorThe current fixed positioning of the loading indicator could cause issues with multiple tables on the same page. Consider using relative positioning within the table container.
- <div className="fixed bottom-0 left-0 right-0 py-2 bg-background/90 backdrop-blur-sm border-t border-border"> + <div className="sticky bottom-0 left-0 right-0 py-2 bg-background/90 backdrop-blur-sm border-t border-border">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/dashboard/app/(app)/audit/[bucket]/components/table/log-footer.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/filters/components/custom-date-filter.tsx(3 hunks)apps/dashboard/app/(app)/logs/components/table/log-details/components/request-response-details.tsx(3 hunks)apps/dashboard/components/virtual-table.tsx(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/app/(app)/logs/components/table/log-details/components/request-response-details.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
apps/dashboard/components/virtual-table.tsx
[error] 68-68: Invalid typeof comparison value: this expression is not a string literal
not a string literal
Unsafe fix: Compare the result of typeof with a valid type name
(lint/suspicious/useValidTypeof)
🔇 Additional comments (6)
apps/dashboard/app/(app)/logs/components/filters/components/custom-date-filter.tsx (2)
6-6: LGTM! Well-structured interface and imports.
The new interface DatePickerWithRangeProps is well-defined and properly extends React's HTML attributes. The imports are appropriate for the component's functionality.
Also applies to: 8-8, 13-18
81-82: LGTM! Explicit null values for cleared state.
Using null instead of undefined is more explicit and aligns well with the interface type definition.
apps/dashboard/app/(app)/audit/[bucket]/components/table/log-footer.tsx (4)
1-11: LGTM! Clean imports and type definitions.
The imports are well-organized and the Props type is clearly defined.
13-16: LGTM! Clean component structure.
The component follows React best practices with proper prop destructuring and component composition.
66-73: User Agent display needs improvement.
The current implementation truncates the User Agent without a way to view the full content.
82-89: Description field needs formatting improvements.
The description should not be truncated and should support multi-line display per PR feedback.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/dashboard/components/virtual-table.tsx (1)
65-71:⚠️ Potential issueFix scroll lock implementation issues
The current implementation needs improvement for better error handling and type safety.
Apply this diff to fix both issues:
useScrollLock({ autoLock: true, lockTarget: - typeof window !== "undefined" - ? (document.querySelector("#layout-wrapper") as HTMLElement) - : undefined, + typeof window !== 'undefined' + ? document.querySelector("#layout-wrapper") || document.body + : undefined, });
🧹 Nitpick comments (7)
apps/dashboard/app/(app)/logs/components/filters/components/custom-date-filter.tsx (3)
81-82: Consider adding validation for date range boundaries.While using
nullfor cleared dates is more explicit, consider adding validation to ensure:
- Start time is not after end time
- Date ranges don't exceed reasonable limits (e.g., max 30 days)
setSearchParams({ - startTime: null, - endTime: null, + startTime: null, + endTime: null, + validate: () => { + if (!startTime || !endTime) return true; + const diffInDays = Math.abs(endTime - startTime) / (1000 * 60 * 60 * 24); + return diffInDays <= 30; + } });
122-122: Consider adding error feedback for invalid date selections.While preventing future date selection is good, consider adding visual feedback or a tooltip to explain why certain dates are disabled.
disabled={(date) => { const isDisabled = date > new Date(); + // Add aria-label for accessibility + if (isDisabled) { + date.setAttribute('aria-label', 'Future dates cannot be selected'); + } return isDisabled; }}
Line range hint
20-165: Consider implementing a reusable date filter hook.The component handles complex date filtering logic that could be useful in other parts of the application. Consider extracting the core date handling logic into a custom hook for reusability.
Example structure:
// useDateRangeFilter.ts export function useDateRangeFilter(initialParams: DateRangeParams) { // Move state and date handling logic here return { dateRange, timeRange, handleDateChange, handleTimeChange, // ... other utilities }; }This would:
- Improve maintainability
- Make the logic reusable across different features
- Simplify testing
apps/dashboard/components/virtual-table.tsx (4)
74-100: Optimize cleanup handling in resize effectThe resize handling implementation is good, but we can optimize the cleanup by combining the cleanup operations.
useEffect(() => { const calculateHeight = () => { if (!containerRef.current) return; const rect = containerRef.current.getBoundingClientRect(); const headerHeight = HEADER_HEIGHT + TABLE_BORDER_THICKNESS; const availableHeight = window.innerHeight - rect.top - headerHeight; setFixedHeight(Math.max(availableHeight, 0)); }; calculateHeight(); const resizeObserver = new ResizeObserver(calculateHeight); const cleanup = () => { resizeObserver.disconnect(); window.removeEventListener("resize", calculateHeight); }; window.addEventListener("resize", calculateHeight); if (containerRef.current) { resizeObserver.observe(containerRef.current); } - return () => { - resizeObserver.disconnect(); - window.removeEventListener("resize", calculateHeight); - }; + return cleanup; }, []);
38-38: Consider making THROTTLE_DELAY configurableThe throttle delay is hardcoded to 350ms. Consider making this configurable through props to allow adjustment based on use case.
+ export type VirtualTableProps<T> = { + // ... existing props + throttleDelay?: number; + }; - const THROTTLE_DELAY = 350; + const DEFAULT_THROTTLE_DELAY = 350; export function VirtualTable<T>({ // ... existing props + throttleDelay = DEFAULT_THROTTLE_DELAY, }: VirtualTableProps<T>) { // ... rest of the implementation const throttledLoadMore = useCallback( throttle( () => { if (onLoadMore) { onLoadMore(); } }, - THROTTLE_DELAY, + throttleDelay, { leading: true, trailing: false }, ), - [onLoadMore], + [onLoadMore, throttleDelay], );Also applies to: 102-114
141-147: Extract scroll threshold multiplier as a constantThe scroll threshold calculation uses a magic number (3) for row multiplication. This should be extracted as a named constant for better maintainability.
+ const SCROLL_THRESHOLD_ROW_MULTIPLIER = 3; if ( !isLoading && !isFetchingNextPage && lastItem.index >= data.length - 1 - instance.options.overscan && - scrollOffset >= scrollElement.scrollHeight - rowHeight * 3 + scrollOffset >= scrollElement.scrollHeight - rowHeight * SCROLL_THRESHOLD_ROW_MULTIPLIER ) { throttledLoadMore(); }
323-330: Enhance loading indicator accessibilityThe loading indicator should have proper ARIA attributes for better screen reader support.
- {isFetchingNextPage && ( + {isFetchingNextPage && ( <div className="fixed bottom-0 left-0 right-0 py-2 bg-background/90 backdrop-blur-sm border-t border-border" + role="status" + aria-live="polite" > <div className="flex items-center justify-center gap-2 text-sm text-accent-11"> <div className="h-1.5 w-1.5 rounded-full bg-accent-11 animate-pulse" + aria-hidden="true" /> - Loading more data + <span>Loading more data</span> </div> </div> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/dashboard/app/(app)/logs/components/filters/components/custom-date-filter.tsx(4 hunks)apps/dashboard/components/virtual-table.tsx(8 hunks)apps/planetfall/next-env.d.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/planetfall/next-env.d.ts
🔇 Additional comments (3)
apps/dashboard/app/(app)/logs/components/filters/components/custom-date-filter.tsx (2)
6-18: LGTM! Well-structured interface and imports.
The new interface DatePickerWithRangeProps properly types the component props and follows TypeScript best practices.
30-33: Consider timezone handling in date operations.
While the state initialization is correct, the component might benefit from explicit timezone handling to ensure consistent behavior across different user locations. Consider using UTC for internal operations and converting to local time only for display.
Let's check for any timezone-related issues in the codebase:
apps/dashboard/components/virtual-table.tsx (1)
2-2: Verify throttle implementation
The throttle utility is being imported from local utils. Let's verify its implementation to ensure it properly handles cleanup and edge cases.
✅ Verification successful
Let me gather more information about the throttle implementation to ensure it properly handles cleanup and edge cases.
Let me get the remaining part of the throttle implementation to verify the cleanup and edge cases handling.
Throttle implementation is well-implemented and safe to use
The throttle utility implementation is robust and includes all necessary features:
- Proper cleanup through
cancelmethod that clears timeout and resets state - Edge case handling with
leadingandtrailingoptions - Memory leak prevention by clearing timeouts
- Type-safe implementation with proper TypeScript generics
- Proper debounce timing with
remainingWaitcalculations - Flush capability to force execution of pending calls
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check throttle implementation
rg -A 10 "export.*function.*throttle"
Length of output: 699
Script:
#!/bin/bash
# Get the complete throttle implementation
rg -A 50 "export.*function.*throttle" apps/dashboard/lib/utils.ts
Length of output: 1516
Script:
#!/bin/bash
# Get the complete throttle implementation including the shouldInvoke function
rg -B 10 -A 50 "function shouldInvoke" apps/dashboard/lib/utils.ts
Length of output: 949
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
Release Notes
New Features
Filterscomponent for enhanced filtering of audit logs, including bucket selection and user filters.AuditLogTableClientfor fetching and displaying logs with pagination support.ClearButtonfor clearing filter selections.RootKeyFilterandUserFiltercomponents for filtering based on workspace and user data.Improvements
DatePickerWithRangeto accept initial date parameters for filtering.RequestResponseDetailscomponent for improved user feedback.LogHeaderandLogFootercomponents for better organization of log details.Bug Fixes
Removals
RowandFilterSinglecomponents to streamline the codebase.