feat: Logs v2 design phase #2789
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive update to the dashboard application, focusing on enhancing the logs and virtual table components. The changes span multiple files across different directories, introducing new components for log visualization, filtering, and table rendering. Key additions include a new logs view (logs-v2), virtualized table implementation, and several utility functions and icons to support these features. Changes
Suggested Labels
Suggested ReviewersThank 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! 🙏 |
ogzhanolguncu
requested review from
perkinsjr,
chronark,
mcstepp and
MichaelUnkey
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🔭 Outside diff range comments (1)
apps/dashboard/app/(app)/logs/components/table/logs-table.tsx (1)
Line range hint
47-64: Refactor status-based styling logic for better maintainability.The current implementation has several areas for improvement:
- Status code ranges are duplicated between functions
- Magic numbers are used for HTTP status checks
- No handling for 3xx status codes
Consider this refactoring:
+ const HTTP_STATUS = { + SUCCESS: { min: 200, max: 299 }, + REDIRECT: { min: 300, max: 399 }, + CLIENT_ERROR: { min: 400, max: 499 }, + SERVER_ERROR: { min: 500, max: 599 }, + } as const; + + const getStatusType = (status: number) => { + if (status >= HTTP_STATUS.SERVER_ERROR.min) return 'error'; + if (status >= HTTP_STATUS.CLIENT_ERROR.min) return 'warning'; + if (status >= HTTP_STATUS.REDIRECT.min) return 'redirect'; + return 'success'; + }; const getRowClassName = (log: Log) => { + const type = getStatusType(log.response_status); + return cn({ + "bg-amber-2 text-amber-11 hover:bg-amber-3": type === 'warning', + "bg-red-2 text-red-11 hover:bg-red-3": type === 'error', + }); - return cn({ - "bg-amber-2 text-amber-11 hover:bg-amber-3": - log.response_status >= 400 && log.response_status < 500, - "bg-red-2 text-red-11 hover:bg-red-3": log.response_status >= 500, - }); }; const getSelectedClassName = (log: Log, isSelected: boolean) => { if (!isSelected) return ""; + const type = getStatusType(log.response_status); + return cn({ + "bg-background-subtle/90": type === 'success', + "bg-amber-3": type === 'warning', + "bg-red-3": type === 'error', + }); - return cn({ - "bg-background-subtle/90": log.response_status >= 200 && log.response_status < 300, - "bg-amber-3": log.response_status >= 400 && log.response_status < 500, - "bg-red-3": log.response_status >= 500, - }); };
🧹 Nitpick comments (40)
apps/dashboard/app/(app)/logs-v2/page.tsx (2)
13-16: Optimize database query by selecting only required fields.Consider optimizing the database query by explicitly selecting only the fields needed (
betaFeatures).const workspace = await db.query.workspaces.findFirst({ where: (table, { and, eq, isNull }) => and(eq(table.tenantId, tenantId), isNull(table.deletedAt)), + columns: { + betaFeatures: true + } });
25-36: Consider improvements to component structure and routing.A few suggestions to enhance maintainability:
- Consider using route constants instead of hard-coded paths
- The outer div could benefit from semantic HTML or styling classes
+ const ROUTES = { + LOGS: '/logs-v2' + } as const; const LogsContainerPage = () => { return ( - <div> + <main className="min-h-screen"> <Navbar> <Navbar.Breadcrumbs icon={<Layers3 />}> - <Navbar.Breadcrumbs.Link href="/logs-v2">Logs</Navbar.Breadcrumbs.Link> + <Navbar.Breadcrumbs.Link href={ROUTES.LOGS}>Logs</Navbar.Breadcrumbs.Link> </Navbar.Breadcrumbs> </Navbar> <LogsClient /> - </div> + </main> ); };apps/dashboard/app/(app)/logs/components/table/logs-table.tsx (1)
Line range hint
17-45: Consider optimizing column definitions for better UX.A few suggestions to enhance the table's usability:
- The path column's percentage-based width ("20%") might cause layout shifts in a virtual table. Consider using a fixed width or flex units.
- The response column could benefit from a more sophisticated truncation with a tooltip or expandable view for long content.
Consider this improvement for the columns:
{ key: "path", - width: "20%", + width: "250px", // or "0.5fr" for flex-based sizing header: "Path", render: (log) => ( <div className="flex items-center gap-2"> <Badge className="bg-background border border-solid border-border text-current hover:bg-transparent"> {log.method} </Badge> {log.path} </div> ), }, { key: "response", header: "Response Body", width: "1fr", - render: (log) => <span className="truncate">{log.response_body}</span>, + render: (log) => ( + <span className="truncate" title={log.response_body}> + {log.response_body} + </span> + ), },apps/dashboard/components/virtual-table/constants.ts (1)
3-10: Consider optimizing performance-related constantsThe configuration values look reasonable, but consider these performance optimizations:
loadingRows: 50might cause unnecessary initial render overhead. Consider reducing to 20-30 rows for faster initial load.throttleDelay: 350mscould make scrolling feel less responsive. Consider reducing to 150-200ms for modern devices.export const DEFAULT_CONFIG: TableConfig = { rowHeight: 26, - loadingRows: 50, + loadingRows: 25, overscan: 5, tableBorder: 1, - throttleDelay: 350, + throttleDelay: 150, headerHeight: 40, } as const;apps/dashboard/components/virtual-table/components/loading-row.tsx (1)
3-18: Optimize performance and enhance type safetyConsider these improvements:
- Memoize the component to prevent unnecessary rerenders
- Move grid template calculation to a separate function
- Add explicit type for column keys
+import { memo } from "react"; + +const getGridTemplate = (columns: Column<unknown>[]) => + columns.map((col) => col.width).join(" "); + -export const LoadingRow = <TTableItem,>({ +export const LoadingRow = memo(<TTableItem,>({ columns, }: { columns: Column<TTableItem>[]; }) => ( <div className="grid text-sm animate-pulse mt-1" - style={{ gridTemplateColumns: columns.map((col) => col.width).join(" ") }} + style={{ gridTemplateColumns: getGridTemplate(columns) }} > {columns.map((column) => ( <div key={column.key} className="px-2"> <div className="h-4 bg-accent-6 rounded" /> </div> ))} </div> -); +)); + +LoadingRow.displayName = "LoadingRow";apps/dashboard/components/virtual-table/types.ts (1)
9-16: Add JSDoc comments to document TableConfig propertiesThe TableConfig type would benefit from documentation explaining the purpose and expected values of each configuration option.
Add JSDoc comments like this:
+/** + * Configuration options for the virtual table + */ export type TableConfig = { + /** Height of each table row in pixels */ rowHeight: number; + /** Number of placeholder rows to show during loading */ loadingRows: number; + /** Number of extra rows to render above/below the visible area */ overscan: number; + /** Border width in pixels */ tableBorder: number; + /** Delay in milliseconds for throttling scroll events */ throttleDelay: number; + /** Height of the table header in pixels */ headerHeight: number; };internal/icons/src/index.ts (1)
20-28: Consider maintaining alphabetical order and consistent namingThe new icon exports break the alphabetical ordering. Also, consider differentiating similar icons like
triangle-warningandtriangle-warning-2more meaningfully.Consider:
- Reordering exports alphabetically
- Renaming
triangle-warning-2to better reflect its specific use caseinternal/icons/src/icons/grid.tsx (1)
15-26: Add accessibility attributes and optimize SVGThe Grid icon implementation could be enhanced with accessibility attributes and SVG optimization.
Consider these improvements:
export const Grid: React.FC<IconProps> = (props) => { return ( <svg {...props} + aria-hidden="true" + role="img" + aria-label="Grid view" height="18" width="18" viewBox="0 0 18 18" xmlns="http://www.w3.org/2000/svg" > - <g fill="currentColor"> + <g> <rect height="6" width="6" fill="currentColor" rx="1.75" ry="1.75" x="2" y="2" /> <rect height="6" width="6" fill="currentColor" rx="1.75" ry="1.75" x="10" y="2" /> <rect height="6" width="6" fill="currentColor" rx="1.75" ry="1.75" x="2" y="10" /> <rect height="6" width="6" fill="currentColor" rx="1.75" ry="1.75" x="10" y="10" /> </g> </svg> ); };Note: Remove redundant
fill="currentColor"from thegelement as it's already specified in therectelements.apps/dashboard/app/(app)/logs-v2/components/charts/util.ts (2)
15-17: Consider parameterizing mock data rangesThe hardcoded ranges for random data generation (50+20 for success, 30 for error, 25 for warning) might not represent realistic scenarios across different use cases.
Consider making these configurable:
- const success = Math.floor(Math.random() * 50) + 20; - const error = Math.floor(Math.random() * 30); - const warning = Math.floor(Math.random() * 25); + const success = Math.floor(Math.random() * successRange.max) + successRange.min; + const error = Math.floor(Math.random() * errorRange.max); + const warning = Math.floor(Math.random() * warningRange.max);
19-28: Optimize data point creationFor large datasets (e.g., small intervals over many hours), consider using array initialization with map for better performance.
- data.push({ - x: Math.floor(timestamp.getTime()), - displayX: timestamp.toISOString(), - originalTimestamp: timestamp.toISOString(), - success, - error, - warning, - total: success + error + warning, - }); + return Array.from({ length: points }, (_, i) => { + const timestamp = new Date(now.getTime() - (points - 1 - i) * intervalMinutes * 60 * 1000); + const success = Math.floor(Math.random() * 50) + 20; + const error = Math.floor(Math.random() * 30); + const warning = Math.floor(Math.random() * 25); + return { + x: Math.floor(timestamp.getTime()), + displayX: timestamp.toISOString(), + originalTimestamp: timestamp.toISOString(), + success, + error, + warning, + total: success + error + warning, + }; + });apps/dashboard/app/(app)/logs-v2/components/logs-client.tsx (1)
10-34: Consider performance and error handling improvementsA few suggestions to enhance the component:
- Wrap child components with error boundaries
- Memoize child components to prevent unnecessary re-renders
- Add loading states for async operations
Example implementation:
+import { memo } from 'react'; +import { ErrorBoundary } from './error-boundary'; -export const LogsClient = () => { +export const LogsClient = memo(() => { const [selectedLog, setSelectedLog] = useState<Log | null>(null); const [tableDistanceToTop, setTableDistanceToTop] = useState(0); + const [isLoading, setIsLoading] = useState(false); // ... existing callbacks ... return ( <> - <LogsFilters /> - <LogsChart onMount={handleDistanceToTop} /> - <LogsTable onLogSelect={handleLogSelection} selectedLog={selectedLog} /> + <ErrorBoundary fallback={<div>Error loading filters</div>}> + <LogsFilters /> + </ErrorBoundary> + <ErrorBoundary fallback={<div>Error loading chart</div>}> + <LogsChart onMount={handleDistanceToTop} /> + </ErrorBoundary> + <ErrorBoundary fallback={<div>Error loading table</div>}> + <LogsTable + onLogSelect={handleLogSelection} + selectedLog={selectedLog} + isLoading={isLoading} + /> + </ErrorBoundary> <LogDetails log={selectedLog} onClose={() => handleLogSelection(null)} distanceToTop={tableDistanceToTop} /> </> ); -}; +});internal/icons/src/icons/xmark.tsx (1)
15-42: Enhance accessibility and reusabilityThe icon component could benefit from:
- ARIA attributes for accessibility
- Configurable dimensions
-export const XMark: React.FC<IconProps> = (props) => { +export const XMark: React.FC<IconProps> = ({ width = 18, height = 18, title, ...props }) => { return ( <svg {...props} - height="18" - width="18" + height={height} + width={width} viewBox="0 0 18 18" xmlns="http://www.w3.org/2000/svg" + role="img" + aria-hidden={!title} + aria-labelledby={title ? 'xmark-title' : undefined} > + {title && <title id="xmark-title">{title}</title>} <g fill="currentColor"> // ... existing paths ... </g> </svg> ); };internal/icons/src/icons/circle-carret-right.tsx (1)
15-15: Fix typo in component name: "Carret" should be "Caret".The word "caret" is misspelled in the component name. This should be corrected for consistency and accuracy.
-export const CircleCarretRight: React.FC<IconProps> = (props) => { +export const CircleCaretRight: React.FC<IconProps> = (props) => {apps/dashboard/app/(app)/logs-v2/query-state.ts (2)
11-14: Export TIMELINE_OPTIONS constant for consistency.Since this constant is used to define the Timeline type, it should be exported like STATUSES for consistency and reusability.
-const TIMELINE_OPTIONS = ["1h", "3h", "6h", "12h", "24h"] as const; +export const TIMELINE_OPTIONS = ["1h", "3h", "6h", "12h", "24h"] as const;
16-24: Enhance type safety for HTTP methods and time range.Consider these improvements:
- Type HTTP methods using a union type
- Make time range fields required for better predictability
+const HTTP_METHODS = ["GET", "POST", "PUT", "DELETE", "PATCH"] as const; +type HttpMethod = (typeof HTTP_METHODS)[number]; export type QuerySearchParams = { host: string | null; requestId: string | null; - method: string | null; + method: HttpMethod | null; path: string | null; responseStatus: ResponseStatus[]; - startTime?: number | null; - endTime?: number | null; + startTime: number | null; + endTime: number | null; };internal/icons/src/icons/refresh-3.tsx (1)
15-15: Consider a more descriptive component name.The name "Refresh3" is not very descriptive. Consider a more semantic name that describes the visual style or purpose of this specific refresh icon.
-export const Refresh3: React.FC<IconProps> = (props) => { +export const RefreshCircular: React.FC<IconProps> = (props) => {internal/icons/src/icons/bars-filter.tsx (1)
15-55: Consider optimizing SVG pathsWhile the implementation is correct, the SVG could be optimized by combining the three lines into a single path element, reducing the DOM size.
- <g fill="currentColor"> - <line - fill="none" - stroke="currentColor" - strokeLinecap="round" - strokeLinejoin="round" - strokeWidth="1.5" - x1="5.25" - x2="12.75" - y1="9" - y2="9" - /> - <line - fill="none" - stroke="currentColor" - strokeLinecap="round" - strokeLinejoin="round" - strokeWidth="1.5" - x1="2.75" - x2="15.25" - y1="4.25" - y2="4.25" - /> - <line - fill="none" - stroke="currentColor" - strokeLinecap="round" - strokeLinejoin="round" - strokeWidth="1.5" - x1="8" - x2="10" - y1="13.75" - y2="13.75" - /> + <g fill="none" stroke="currentColor" strokeLinecap="round" strokeLinejoin="round" strokeWidth="1.5"> + <path d="M5.25 9h7.5 M2.75 4.25h12.5 M8 13.75h2" /> </g>apps/dashboard/app/(app)/logs-v2/components/filters/index.tsx (1)
12-13: Consider responsive design for mobile viewsThe current layout might not work well on smaller screens. Consider adding responsive classes to handle mobile views gracefully.
- <div className="flex flex-col border-b border-gray-4 "> - <div className="px-3 py-2 w-full justify-between flex items-center min-h-10"> + <div className="flex flex-col border-b border-gray-4"> + <div className="px-3 py-2 w-full justify-between flex flex-wrap gap-y-2 items-center min-h-10">apps/dashboard/components/virtual-table/hooks/useVirtualData.ts (2)
6-20: Consider adding error handling and loading statesThe hook could benefit from additional error handling and loading state management:
- Add error state handling for failed load attempts
- Include a way to reset error states
- Add loading indicator position calculation
export const useVirtualData = <T>({ data, isLoading, config, onLoadMore, isFetchingNextPage, parentRef, + onError?: (error: Error) => void, }: { data: T[]; isLoading: boolean; config: TableConfig; onLoadMore?: () => void; isFetchingNextPage?: boolean; parentRef: React.RefObject<HTMLDivElement>; + onError?: (error: Error) => void; })
52-62: Add debounce option for scroll handlingThe current implementation only uses throttling. Consider adding a debounce option for cases where immediate response isn't critical.
const scrollOffset = scrollElement.scrollTop + scrollElement.clientHeight; const scrollThreshold = scrollElement.scrollHeight - config.rowHeight * 3; +const shouldLoadMore = + !isLoading && + !isFetchingNextPage && + lastItem.index >= data.length - 1 - instance.options.overscan && + scrollOffset >= scrollThreshold; -if ( - !isLoading && - !isFetchingNextPage && - lastItem.index >= data.length - 1 - instance.options.overscan && - scrollOffset >= scrollThreshold -) { - throttledLoadMore(); -} +if (shouldLoadMore) { + config.useDebounce ? debouncedLoadMore() : throttledLoadMore(); +}internal/icons/src/icons/calendar.tsx (1)
15-60: Enhance accessibility and prop types for the Calendar iconThe icon component could benefit from better accessibility support and more specific prop types.
+type CalendarIconProps = IconProps & { + 'aria-label'?: string; + 'aria-hidden'?: boolean; +}; -export const Calendar: React.FC<IconProps> = (props) => { +export const Calendar: React.FC<CalendarIconProps> = ({ + 'aria-label': ariaLabel = 'Calendar', + 'aria-hidden': ariaHidden = true, + ...props +}) => { return ( <svg {...props} + role="img" + aria-label={ariaHidden ? undefined : ariaLabel} + aria-hidden={ariaHidden} height="18" width="18" viewBox="0 0 18 18"apps/dashboard/app/(app)/logs-v2/components/table/log-details/index.tsx (1)
13-14: Consider making panel dimensions configurableThe panel dimensions are hardcoded. Consider making them configurable through props or theme configuration.
-const PANEL_MAX_WIDTH = 600; -const PANEL_MIN_WIDTH = 400; +interface PanelConfig { + maxWidth?: number; + minWidth?: number; +} + +const DEFAULT_PANEL_CONFIG: PanelConfig = { + maxWidth: 600, + minWidth: 400, +};apps/dashboard/app/(app)/logs-v2/components/table/log-details/components/log-section.tsx (1)
13-23: Consider adding fallback for Clipboard APIThe Clipboard API might not be available in all browsers. Consider adding a fallback mechanism.
const handleClick = () => { + const content = getFormattedContent(details); + if (!navigator.clipboard) { + const textArea = document.createElement('textarea'); + textArea.value = content; + document.body.appendChild(textArea); + textArea.select(); + try { + document.execCommand('copy'); + toast.success(`${title} copied to clipboard`); + } catch (error) { + console.error("Failed to copy to clipboard:", error); + toast.error("Failed to copy to clipboard"); + } + document.body.removeChild(textArea); + return; + } navigator.clipboard - .writeText(getFormattedContent(details)) + .writeText(content)apps/dashboard/app/(app)/logs-v2/components/table/utils.ts (1)
73-75: Consider adding timestamp variation in mock dataCurrently, all logs will have timestamps very close to each other. Consider spreading them across a reasonable time range.
export const generateMockLogs = (count: number, overrides = {}) => { - return Array.from({ length: count }, () => generateMockLog(overrides)); + const now = Date.now(); + return Array.from({ length: count }, (_, index) => { + const timeOffset = index * 60000; // 1 minute apart + return generateMockLog({ + ...overrides, + time: Math.floor((now - timeOffset) / 1000), + }); + }); };apps/dashboard/app/(app)/logs-v2/components/table/log-details/resizable-panel.tsx (1)
23-23: Consider using number type for width stateConverting between string and number types for width can be avoided by maintaining the state as a number.
- const [width, setWidth] = useState<string>(String(style?.width)); + const [width, setWidth] = useState<number>( + typeof style?.width === 'number' ? style.width : MIN_DRAGGABLE_WIDTH + );Then update the style prop usage:
- style={{ ...style, width, right: 0, position: "fixed" }} + style={{ ...style, width: `${width}px`, right: 0, position: "fixed" }}apps/dashboard/components/virtual-table/components/table-row.tsx (2)
36-37: Consider using React refs instead of direct DOM manipulationThe use of
document.activeElementand direct DOM manipulation could be replaced with React refs for better maintainability and to follow React's principles.- const activeElement = document.activeElement as HTMLElement; - activeElement?.blur(); + if (event.currentTarget instanceof HTMLElement) { + event.currentTarget.blur(); + }
77-80: Add aria-label for better accessibilityThe column cells could benefit from aria-labels to improve screen reader experience.
- <div key={column.key} className="truncate flex items-center h-full"> + <div + key={column.key} + className="truncate flex items-center h-full" + aria-label={`${column.header}: ${column.render(item)}`} + >apps/dashboard/app/(app)/audit/components/table/columns.tsx (2)
Line range hint
37-39: Add aria-label to icons for better accessibilityIcons should have aria-labels to improve screen reader experience.
- <KeySquare className="w-4 h-4" /> + <KeySquare className="w-4 h-4" aria-label="API Key" />
16-16: Standardize padding classes across columnsThe padding classes are inconsistent across different columns. Consider standardizing them for better maintainability.
Consider using a constant for consistent padding:
const CELL_PADDING = "px-2";Then apply it consistently across all column renders.
Also applies to: 30-30, 65-65, 77-77
apps/dashboard/app/(app)/logs-v2/components/table/log-details/components/request-response-details.tsx (1)
21-39: Optimize isNonEmpty function performanceThe function could be optimized by reducing type checks and avoiding unnecessary iterations.
const isNonEmpty = (content: unknown): boolean => { if (content === undefined || content === null) { return false; } + if (typeof content === "string") { + return content.trim().length > 0; + } + if (Array.isArray(content)) { - return content.some((item) => item !== null && item !== undefined); + return content.length > 0; } if (typeof content === "object" && content !== null) { - return Object.values(content).some((value) => value !== null && value !== undefined); + return Object.keys(content).length > 0; } - if (typeof content === "string") { - return content.trim().length > 0; - } - return Boolean(content); };apps/dashboard/app/(app)/logs-v2/components/table/log-details/components/log-footer.tsx (1)
14-14: Consider moving DEFAULT_OUTCOME to constants file.For better maintainability and reusability, consider moving the
DEFAULT_OUTCOMEconstant to the constants file along with other state-related constants.apps/dashboard/app/(app)/logs-v2/utils.ts (2)
74-85: Enhance error handling in safeParseJson.Consider providing more specific error information instead of a generic "Invalid JSON format" message.
Apply this diff to improve error handling:
export const safeParseJson = (jsonString?: string | null) => { if (!jsonString) { return null; } try { return JSON.parse(jsonString); } catch (error) { - console.error("Cannot parse JSON:", jsonString); - return "Invalid JSON format"; + const errorMessage = error instanceof Error ? error.message : "Unknown parsing error"; + console.error("Cannot parse JSON:", { error: errorMessage, input: jsonString }); + return `Invalid JSON format: ${errorMessage}`; } };
87-89: Consider using more descriptive constant names.The time constants could be more descriptive about their purpose in the context of time series data.
Apply this diff to improve clarity:
-export const HOUR_IN_MS = 60 * 60 * 1000; -const DAY_IN_MS = 24 * HOUR_IN_MS; -const WEEK_IN_MS = 7 * DAY_IN_MS; +export const TIMESERIES_HOUR_INTERVAL = 60 * 60 * 1000; +const TIMESERIES_DAY_INTERVAL = 24 * TIMESERIES_HOUR_INTERVAL; +const TIMESERIES_WEEK_INTERVAL = 7 * TIMESERIES_DAY_INTERVAL;apps/dashboard/components/virtual-table/index.tsx (2)
52-64: Consider memoizing handleRowClick callbackThe
handleRowClickfunction could be memoized usinguseCallbackto prevent unnecessary re-renders of child components that receive this as a prop.- const handleRowClick = useCallback( + const handleRowClick = useCallback( (item: T) => { if (onRowClick) { onRowClick(item); setTableDistanceToTop( (parentRef.current?.getBoundingClientRect().top ?? 0) + window.scrollY - config.tableBorder, ); } }, - [onRowClick, config.tableBorder], + [onRowClick, config.tableBorder, parentRef], );
98-138: Consider extracting loading row rendering logicThe loading row rendering logic could be extracted into a separate component for better maintainability.
+ const LoadingRowRenderer = ({ virtualRow, columns }: { virtualRow: VirtualItem, columns: Column<T>[] }) => ( + <div + key={virtualRow.key} + style={{ + position: "absolute", + top: `${virtualRow.start}px`, + width: "100%", + }} + > + <LoadingRow columns={columns} /> + </div> + ); {virtualizer.getVirtualItems().map((virtualRow) => { if (isLoading) { - return ( - <div - key={virtualRow.key} - style={{ - position: "absolute", - top: `${virtualRow.start}px`, - width: "100%", - }} - > - <LoadingRow columns={columns} /> - </div> - ); + return <LoadingRowRenderer virtualRow={virtualRow} columns={columns} />; } // ... rest of the codeapps/dashboard/app/(app)/logs-v2/components/charts/index.tsx (2)
82-82: Remove biome-ignore commentThe biome-ignore comment for array index usage as key could be avoided by using the timestamp as a unique key instead.
- // biome-ignore lint/suspicious/noArrayIndexKey: use of index is acceptable here. - <div key={i}>{formatTimestampLabel(time)}</div> + <div key={time.getTime()}>{formatTimestampLabel(time)}</div>
89-89: Consider making Y-axis scale configurableThe Y-axis domain calculation uses a hardcoded multiplier of 1.5 for the maximum value. This should be configurable for different data distributions.
- <YAxis domain={[0, (dataMax: number) => dataMax * 1.5]} hide /> + <YAxis domain={[0, (dataMax: number) => dataMax * (config.yAxisScale ?? 1.5)]} hide />apps/dashboard/app/(app)/logs-v2/components/table/logs-table.tsx (1)
189-189: Avoid using percentage-based widthsUsing percentage-based widths (15%) for table columns can lead to inconsistent layouts. Consider using fixed widths or flex-based solutions.
- width: "15%", + width: "200px", // or another appropriate fixed widthapps/dashboard/components/ui/chart.tsx (1)
220-235: Enhance tooltip value formattingThe number formatting could be more flexible. Consider:
- Adding a prop for custom number formatting
- Supporting different locale options
- <span className="font-mono tabular-nums text-accent-12"> - {item.value.toLocaleString()} - </span> + <span className="font-mono tabular-nums text-accent-12"> + {typeof item.value === 'number' + ? item.value.toLocaleString(undefined, formatter?.options) + : item.value} + </span>apps/engineering/content/design/icons.mdx (1)
49-51: Minor grammar improvement neededAdd a comma after "In most cases" for better readability.
- "A className applied to the icon to override the styling in edge cases. In most cases you should not change the size of the icon.", + "A className applied to the icon to override the styling in edge cases. In most cases, you should not change the size of the icon.",🧰 Tools
🪛 LanguageTool
[formatting] ~49-~49: Insert a comma after ‘cases’: “In most cases,”?
Context: ... to override the styling in edge cases. In most cases you should not change the size of the i...(IN_MOST_CASES_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
apps/dashboard/app/(app)/audit/components/table/audit-log-table-client.tsx(1 hunks)apps/dashboard/app/(app)/audit/components/table/columns.tsx(4 hunks)apps/dashboard/app/(app)/logs-v2/components/charts/index.tsx(1 hunks)apps/dashboard/app/(app)/logs-v2/components/charts/util.ts(1 hunks)apps/dashboard/app/(app)/logs-v2/components/filters/index.tsx(1 hunks)apps/dashboard/app/(app)/logs-v2/components/logs-client.tsx(1 hunks)apps/dashboard/app/(app)/logs-v2/components/table/hooks.ts(1 hunks)apps/dashboard/app/(app)/logs-v2/components/table/log-details/components/log-footer.tsx(1 hunks)apps/dashboard/app/(app)/logs-v2/components/table/log-details/components/log-header.tsx(1 hunks)apps/dashboard/app/(app)/logs-v2/components/table/log-details/components/log-meta.tsx(1 hunks)apps/dashboard/app/(app)/logs-v2/components/table/log-details/components/log-section.tsx(1 hunks)apps/dashboard/app/(app)/logs-v2/components/table/log-details/components/request-response-details.tsx(1 hunks)apps/dashboard/app/(app)/logs-v2/components/table/log-details/index.tsx(1 hunks)apps/dashboard/app/(app)/logs-v2/components/table/log-details/resizable-panel.tsx(1 hunks)apps/dashboard/app/(app)/logs-v2/components/table/logs-table.tsx(1 hunks)apps/dashboard/app/(app)/logs-v2/components/table/utils.ts(1 hunks)apps/dashboard/app/(app)/logs-v2/constants.ts(1 hunks)apps/dashboard/app/(app)/logs-v2/page.tsx(1 hunks)apps/dashboard/app/(app)/logs-v2/query-state.ts(1 hunks)apps/dashboard/app/(app)/logs-v2/utils.ts(1 hunks)apps/dashboard/app/(app)/logs/components/table/logs-table.tsx(1 hunks)apps/dashboard/components/timestamp-info.tsx(3 hunks)apps/dashboard/components/ui/chart.tsx(6 hunks)apps/dashboard/components/virtual-table.tsx(0 hunks)apps/dashboard/components/virtual-table/components/empty-state.tsx(1 hunks)apps/dashboard/components/virtual-table/components/loading-indicator.tsx(1 hunks)apps/dashboard/components/virtual-table/components/loading-row.tsx(1 hunks)apps/dashboard/components/virtual-table/components/table-header.tsx(1 hunks)apps/dashboard/components/virtual-table/components/table-row.tsx(1 hunks)apps/dashboard/components/virtual-table/constants.ts(1 hunks)apps/dashboard/components/virtual-table/hooks/useTableHeight.ts(1 hunks)apps/dashboard/components/virtual-table/hooks/useVirtualData.ts(1 hunks)apps/dashboard/components/virtual-table/index.tsx(1 hunks)apps/dashboard/components/virtual-table/types.ts(1 hunks)apps/engineering/content/design/icons.mdx(2 hunks)internal/icons/src/icons/bars-filter.tsx(1 hunks)internal/icons/src/icons/calendar.tsx(1 hunks)internal/icons/src/icons/circle-carret-right.tsx(1 hunks)internal/icons/src/icons/grid.tsx(1 hunks)internal/icons/src/icons/magnifier.tsx(1 hunks)internal/icons/src/icons/refresh-3.tsx(1 hunks)internal/icons/src/icons/sliders.tsx(1 hunks)internal/icons/src/icons/triangle-warning-2.tsx(1 hunks)internal/icons/src/icons/xmark.tsx(1 hunks)internal/icons/src/index.ts(1 hunks)internal/ui/src/components/button.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- apps/dashboard/components/virtual-table.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/dashboard/app/(app)/logs-v2/constants.ts
🧰 Additional context used
📓 Learnings (2)
apps/dashboard/app/(app)/logs-v2/components/table/log-details/resizable-panel.tsx (1)
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2143
File: apps/dashboard/app/(app)/logs/components/log-details/resizable-panel.tsx:37-49
Timestamp: 2024-12-03T14:23:07.189Z
Learning: In `apps/dashboard/app/(app)/logs/components/log-details/resizable-panel.tsx`, the resize handler is already debounced.
apps/dashboard/app/(app)/logs-v2/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.
🪛 LanguageTool
apps/engineering/content/design/icons.mdx
[formatting] ~49-~49: Insert a comma after ‘cases’: “In most cases,”?
Context: ... to override the styling in edge cases. In most cases you should not change the size of the i...
(IN_MOST_CASES_COMMA)
🪛 Biome (1.9.4)
apps/dashboard/app/(app)/logs-v2/components/table/log-details/components/log-section.tsx
[error] 38-38: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🪛 Gitleaks (8.21.2)
apps/dashboard/app/(app)/logs-v2/components/table/utils.ts
54-54: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Agent Local / test_agent_local
🔇 Additional comments (27)
apps/dashboard/app/(app)/logs-v2/page.tsx (1)
1-9: LGTM! Server directive and imports are well organized.The "use server" directive is correctly placed, and imports are relevant to the component's functionality.
apps/dashboard/app/(app)/logs/components/table/logs-table.tsx (3)
Line range hint
1-11: LGTM! Clean imports and well-structured component setup.The component follows React best practices with clear dependency imports and proper state management.
Line range hint
66-85: LGTM! Well-implemented virtual table integration.The VirtualTable implementation is clean with proper handling of:
- Row selection and details rendering
- Loading states and pagination
- Key extraction for optimization
Line range hint
1-85: Verify alignment with Logs v2 design objectives.The component implementation aligns well with the PR objectives for the logs v2 design phase, particularly in terms of UI enhancements and VirtualTable refactoring. However, please ensure that:
- The color scheme matches the new design system mentioned in the PR objectives
- The component's performance meets expectations with large datasets
Run the following script to verify color token usage consistency:
✅ Verification successful
Color scheme and performance implementation verified
The component's implementation:
- Uses semantic color tokens consistently (amber for 4xx, red for 5xx errors)
- Follows the design system's background layering (bg-background, bg-background-subtle)
- Efficiently applies conditional styles using the cn() utility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of color tokens across the codebase # Search for color-related class names and custom colors rg -g '*.tsx' -g '*.css' '\b(bg-|text-|border-)(red|amber|background)' ./apps/dashboard/Length of output: 14242
apps/dashboard/components/virtual-table/types.ts (1)
18-33: Consider adding validation for certain prop combinationsSome prop combinations might need validation to prevent invalid states.
For example:
selectedItemshould exist indataarrayrenderDetailsrequiresonRowClickisFetchingNextPagerequiresonLoadMoreConsider adding runtime validation or type guards. I can help verify the current usage with this script:
internal/icons/src/icons/circle-carret-right.tsx (1)
15-37: LGTM! Clean SVG implementation with proper theming support.The component follows SVG best practices with proper viewBox, dimensions, and theme compatibility using currentColor.
apps/dashboard/app/(app)/logs-v2/query-state.ts (1)
36-40: LGTM! Clean implementation of the search params hook.The hook provides a simple and effective interface for managing search parameters.
internal/icons/src/icons/magnifier.tsx (1)
15-43: LGTM! Clean and semantic SVG implementation.The magnifier icon follows SVG best practices with proper viewBox, dimensions, and semantic grouping of elements.
internal/icons/src/icons/refresh-3.tsx (1)
15-38: LGTM! Clean SVG implementation.The refresh icon follows SVG best practices with proper viewBox, dimensions, and path definitions.
apps/dashboard/app/(app)/logs-v2/components/table/log-details/components/log-meta.tsx (1)
19-36: LGTM! Well-structured UI with good accessibilityThe implementation includes:
- Proper aria-label for the copy button
- Efficient use of hover states
- Clean layout with appropriate spacing
internal/icons/src/icons/triangle-warning-2.tsx (1)
1-43: LGTM! Well-implemented icon component with proper licensingThe implementation follows SVG best practices with:
- Proper scaling and viewBox
- Dynamic color inheritance
- Clear licensing terms
Remember to comply with Nucleo's license restrictions regarding redistribution and usage.
apps/dashboard/app/(app)/logs-v2/components/table/log-details/components/log-header.tsx (1)
14-41: LGTM! Well-structured layout with proper truncationThe component implements a clean and efficient layout with:
- Proper use of flex containers
- Text truncation for long paths
- Consistent spacing and alignment
internal/icons/src/icons/sliders.tsx (2)
15-86: Implementation looks good!The SVG icon implementation follows React best practices:
- Uses currentColor for dynamic coloring
- Proper SVG attributes and viewBox
- Accessible with proper dimensions
1-11: Verify license compliance with Nucleo IconsPlease ensure that:
- The Nucleo Icons license has been properly purchased
- The usage complies with the redistribution restrictions
✅ Verification successful
✅ License compliance verified for Nucleo Icons
Found proper license file and documentation:
internal/icons/LICENSEcontains the Nucleo Icons licenseapps/engineering/content/design/icons.mdxdocuments the team's process for using Nucleo UI Essential collection🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if Nucleo license exists in the repository fd -g "nucleo*license*" -g "LICENSE*nucleo*" -iLength of output: 152
Script:
#!/bin/bash # Search for license files and documentation about Nucleo fd -i -e md -e txt -e json . | rg -i "nucleo|license" --files-with-matches # Search for Nucleo mentions in package files fd "package.json" -x jq -r '.dependencies + .devDependencies | keys[]' 2>/dev/null | rg -i "nucleo" # Search for any files mentioning Nucleo rg -i "nucleo" -g "!*.{svg,tsx}" -g "!**/dist/**" -g "!**/build/**"Length of output: 915
apps/dashboard/app/(app)/logs-v2/components/table/log-details/resizable-panel.tsx (1)
37-49: 💡 Codebase verificationConsider debouncing the resize handler using the existing utility
The resize handler should be debounced using the
debounceutility fromapps/dashboard/lib/utils.tsto optimize performance during rapid resizing. This follows the pattern established in the v1 implementation.🔗 Analysis chain
Consider debouncing the resize handler
Based on past implementations, the resize handler should be debounced to optimize performance during rapid resizing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if debounce utility is already available in the codebase rg -l "debounce" --type tsLength of output: 365
Script:
#!/bin/bash # Check the v1 implementation and debounce utility ast-grep --pattern 'const handleMouseMove = $_' apps/dashboard/app/(app)/logs/components/log-details/resizable-panel.tsx cat apps/dashboard/lib/utils.ts | grep -A 10 "debounce"Length of output: 303
Script:
#!/bin/bash # Check both v1 and v2 implementations and the debounce utility rg -U "const handleMouseMove" -B 2 -A 10 "apps/dashboard/app/(app)/logs/components/log-details/resizable-panel.tsx" "apps/dashboard/app/(app)/logs-v2/components/table/log-details/resizable-panel.tsx" rg "export .* debounce" -B 0 -A 5 "apps/dashboard/lib/utils.ts"Length of output: 2062
Script:
#!/bin/bash # Find the correct path for v1 implementation fd resizable-panel.tsxLength of output: 185
Script:
#!/bin/bash # Check v1 implementation for debounce usage rg -U "const handleMouseMove" -B 5 -A 15 apps/dashboard/app/(app)/logs/components/table/log-details/resizable-panel.tsxLength of output: 302
⛔ Skipped due to learnings
Learnt from: ogzhanolguncu PR: unkeyed/unkey#2143 File: apps/dashboard/app/(app)/logs/components/log-details/resizable-panel.tsx:37-49 Timestamp: 2024-12-03T14:23:07.189Z Learning: In `apps/dashboard/app/(app)/logs/components/log-details/resizable-panel.tsx`, the resize handler is already debounced.apps/dashboard/app/(app)/audit/components/table/audit-log-table-client.tsx (1)
110-112: LGTM! Props refactoring looks good.The change to nest
loadingRowswithin aconfigobject aligns with component prop standardization.apps/dashboard/components/timestamp-info.tsx (1)
79-79: LGTM! Style and code improvements.The changes improve the component's visual presentation and code quality:
- Simplified setTimeout callback
- Enhanced hover and cursor styles
- Better color contrast with accent colors
Also applies to: 81-81, 83-86
apps/dashboard/app/(app)/logs-v2/utils.ts (3)
4-12: LGTM! Robust error handling implementation.The implementation of
ResponseBodyParseErrorandextractResponseFieldincludes:
- Comprehensive error handling
- Detailed error messages with context
- Type-safe field extraction
- Proper null checks
Also applies to: 14-49
51-72: LGTM! Well-implemented header extraction.The
getRequestHeaderfunction includes:
- Input validation
- Case-insensitive header matching
- Proper array checks
- Clear warning messages
98-136: LGTM! Clear time series granularity logic.The
getTimeseriesGranularityfunction:
- Handles missing parameters gracefully
- Uses clear time range thresholds
- Returns a well-structured config object
internal/ui/src/components/button.tsx (1)
24-24: LGTM! Icon size variant additionThe new icon size variant follows the established pattern and provides consistent sizing for icon buttons.
apps/dashboard/components/virtual-table/index.tsx (1)
44-50: Verify scroll lock target selectorThe scroll lock implementation uses a hardcoded selector
#layout-wrapperwhich could be fragile.Run this script to verify the selector's existence:
✅ Verification successful
The layout-wrapper selector is correctly implemented
The ID is properly defined in
apps/dashboard/app/(app)/layout.tsxand matches the selector used in the virtual table component.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the layout-wrapper ID exists in the codebase rg -l 'id=["'']layout-wrapper["'']' --type html --type tsx --type jsxLength of output: 95
Script:
#!/bin/bash # Search for layout-wrapper ID in all files, including different quotation styles and dynamic assignments rg -i "layout-wrapper" --type-add 'tsx:*.tsx' --type-add 'jsx:*.jsx' # Also search for any Layout component definitions that might set this ID ast-grep --pattern 'const Layout = $_'Length of output: 3443
apps/dashboard/components/ui/chart.tsx (3)
14-14: LGTM! Well-typed addition to ChartConfigThe optional
subLabelproperty is a good addition that enhances the chart's labeling capabilities while maintaining backward compatibility.
107-107: LGTM! Well-typed prop additionThe optional
bottomExplainerprop enhances the tooltip's flexibility while maintaining backward compatibility.
167-168: Consider responsive design implicationsThe width constraints (
sm:w-fit md:w-[256px]) might cause content overflow on very small screens. Consider adding overflow handling for the smallest breakpoint.apps/engineering/content/design/icons.mdx (2)
34-42: LGTM! Comprehensive icon additionsThe new icons align well with the logs page requirements, providing necessary UI elements for filtering and navigation.
60-71: LGTM! Clear example implementationThe example effectively demonstrates icon customization with proper className usage.
apps/dashboard/components/virtual-table/components/loading-indicator.tsx
Show resolved
Hide resolved
apps/dashboard/app/(app)/logs-v2/components/table/log-details/components/log-footer.tsx
Show resolved
Hide resolved
|
@ogzhanolguncu do you want a review and merge this? |
Yes |
|
:ack: , will review later today |
What does this PR do?
This PR adds required design changes for the new logs page and also refactors the VirtualTable component that is being used on the Logs and Audit logs pages.
Also:
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
Release Notes
New Features
Improvements
UI Updates