Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
📝 WalkthroughWalkthroughThis pull request introduces comprehensive changes to the logs and audit components in the dashboard application. The modifications focus on enhancing the functionality, structure, and user experience of log filtering, displaying, and managing components. Key changes include updating import statements, restructuring components, introducing new hooks and utilities for log management, and improving the overall data fetching and rendering mechanisms for logs and audit logs. Changes
Sequence DiagramsequenceDiagram
participant User
participant LogsControls
participant LogsContext
participant LogsTable
participant LogsQuery
User->>LogsControls: Select filters
LogsControls->>LogsContext: Update filters
LogsContext->>LogsQuery: Fetch logs with new filters
LogsQuery-->>LogsTable: Return filtered logs
LogsTable->>User: Display updated logs
Possibly related PRs
Suggested Labels
Suggested Reviewers
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: 1
♻️ Duplicate comments (1)
apps/dashboard/app/(app)/logs/components/charts/hooks/use-fetch-timeseries.ts (1)
44-108:⚠️ Potential issueEnhance error handling and simplify filter processing.
The current implementation logs errors but continues execution, which could lead to incorrect data. Consider throwing errors or providing fallback behavior.
🧹 Nitpick comments (8)
apps/dashboard/app/(app)/logs/page.tsx (1)
10-22: Add error handling for database queries.While the code handles the case of a missing workspace, it should also handle potential database query errors.
Consider wrapping the database query in a try-catch block:
export default async function Page() { const tenantId = getTenantId(); + try { const workspace = await db.query.workspaces.findFirst({ where: (table, { and, eq, isNull }) => and(eq(table.tenantId, tenantId), isNull(table.deletedAt)), }); if (!workspace) { return notFound(); } + } catch (error) { + console.error('Failed to fetch workspace:', error); + throw error; // Let Next.js error boundary handle it + } return <LogsContainerPage />; }apps/dashboard/app/(app)/logs/components/charts/hooks/use-fetch-timeseries.ts (3)
9-10: Consider making the data window configurable and use MS suffix.The hardcoded time window might need to be configurable for different use cases. Also, for better readability, consider using the MS suffix for millisecond values.
-// Duration in milliseconds for historical data fetch window (1 hours) -const TIMESERIES_DATA_WINDOW = 60 * 60 * 1000; +// Duration in milliseconds for historical data fetch window (1 hour) +const TIMESERIES_DATA_WINDOW_MS = 60 * 60 * 1000;
34-42: Remove unnecessary optional chaining.The properties in
paramsare explicitly initialized, making optional chaining unnecessary.- params.status?.filters.push({ + params.status.filters.push({
113-115: Consider making the refetch interval configurable.The 10-second refetch interval is hardcoded. Consider making it configurable based on user needs or system requirements.
+const DEFAULT_REFETCH_INTERVAL_MS = 10_000; + const { data, isLoading, isError } = trpc.logs.queryTimeseries.useQuery(queryParams, { - refetchInterval: queryParams.endTime ? false : 10_000, + refetchInterval: queryParams.endTime ? false : DEFAULT_REFETCH_INTERVAL_MS, });apps/dashboard/app/(app)/logs/components/controls/components/logs-datetime/components/datetime-popover.tsx (2)
13-13: Consolidate duplicate constants.There are two constants (
CUSTOM_OPTION_IDandCUSTOM_DATE_TIME) with the same value (10) used for identifying custom date time options. Consider consolidating them into a single constant to improve maintainability.-const CUSTOM_OPTION_ID = 10; -const CUSTOM_DATE_TIME = 10; +const CUSTOM_DATE_TIME_ID = 10;Also applies to: 77-77
243-265: Consider extracting DateTime configuration into a separate component.The DateTime configuration is complex enough to warrant its own component. This would improve readability and maintainability.
+const DateTimeSelector = ({ + initialStartDate, + initialEndDate, + onDateTimeChange, + handleApplyFilter, +}: { + initialStartDate?: number; + initialEndDate?: number; + onDateTimeChange: (newRange?: Range, newStart?: TimeUnit, newEnd?: TimeUnit) => void; + handleApplyFilter: () => void; +}) => ( + <DateTime + onChange={(newRange, newStart, newEnd) => onDateTimeChange(newRange, newStart, newEnd)} + initialRange={{ + from: initialStartDate ? new Date(initialStartDate) : undefined, + to: initialEndDate ? new Date(initialEndDate) : undefined, + }} + className="gap-3 h-full" + > + <DateTime.Calendar + mode="range" + className="px-3 pt-2.5 pb-3.5 border-b border-gray-4 text-[13px]" + /> + <DateTime.TimeInput type="range" className="px-3.5 h-9 mt-0" /> + <DateTime.Actions className="px-3.5 h-full pb-4"> + <Button + variant="primary" + className="font-sans w-full h-9 rounded-md" + onClick={handleApplyFilter} + > + Apply Filter + </Button> + </DateTime.Actions> + </DateTime> +); // Replace the existing DateTime component with: <DateTimeSelector initialStartDate={initialStartDate} initialEndDate={initialEndDate} onDateTimeChange={onDateTimeChange} handleApplyFilter={handleApplyFilter} />apps/dashboard/components/virtual-table/hooks/useVirtualData.ts (1)
Line range hint
40-72: Consider extracting complex conditions for better readability.While the logic is correct, the nested conditions could be more readable.
Consider this refactoring:
- if ( - !isLoading && - !isFetchingNextPage && - lastItem.index >= totalDataLength - 1 - instance.options.overscan && - scrollOffset >= scrollThreshold - ) { - throttledLoadMore(); - } + const isLoadingMore = isLoading || isFetchingNextPage; + const isNearEnd = lastItem.index >= totalDataLength - 1 - instance.options.overscan; + const hasScrolledNearBottom = scrollOffset >= scrollThreshold; + + if (!isLoadingMore && isNearEnd && hasScrolledNearBottom) { + throttledLoadMore(); + }apps/dashboard/components/virtual-table/index.tsx (1)
Line range hint
71-83: Consider using array length destructuring for cleaner empty check.The empty state check can be more concise.
- if (!isLoading && historicData.length === 0 && realtimeData.length === 0) { + if (!isLoading && [historicData, realtimeData].every(arr => arr.length === 0)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/dashboard/app/(app)/logs/components/charts/hooks/use-fetch-timeseries.ts(1 hunks)apps/dashboard/app/(app)/logs/components/controls/components/logs-datetime/components/datetime-popover.tsx(1 hunks)apps/dashboard/app/(app)/logs/page.tsx(1 hunks)apps/dashboard/components/virtual-table/components/table-row.tsx(2 hunks)apps/dashboard/components/virtual-table/hooks/useVirtualData.ts(2 hunks)apps/dashboard/components/virtual-table/index.tsx(8 hunks)apps/dashboard/lib/trpc/routers/logs/query-distinct-paths.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/dashboard/components/virtual-table/components/table-row.tsx
- apps/dashboard/lib/trpc/routers/logs/query-distinct-paths.ts
🧰 Additional context used
📓 Learnings (1)
apps/dashboard/app/(app)/logs/components/controls/components/logs-datetime/components/datetime-popover.tsx (1)
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2825
File: apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-datetime/index.tsx:0-0
Timestamp: 2025-01-30T20:38:00.058Z
Learning: In the logs dashboard, keyboard shortcuts that toggle UI elements (like popovers) should be implemented in the component that owns the state being toggled, not in the presentational wrapper components. For example, the 'T' shortcut for toggling the datetime filter is implemented in DatetimePopover, not in LogsDateTime.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test GO API Local / test_agent_local
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (11)
apps/dashboard/app/(app)/logs/page.tsx (2)
1-8: LGTM! Imports and server directive are properly configured.The imports are well-organized and all are being utilized in the component.
25-34: LGTM! Component structure is clean and routing is correctly configured.The component follows React best practices and the breadcrumb navigation is properly set to "/logs".
apps/dashboard/app/(app)/logs/components/charts/hooks/use-fetch-timeseries.ts (1)
12-27: LGTM! Well-structured timestamp formatting utility.The function handles all granularity cases correctly with appropriate date formats and proper timezone offset handling.
apps/dashboard/app/(app)/logs/components/controls/components/logs-datetime/components/datetime-popover.tsx (3)
79-88: LGTM! Well-defined types.The types and interfaces are clear, focused, and follow TypeScript best practices.
124-127: LGTM! Keyboard shortcut implementation follows best practices.The keyboard shortcut is correctly implemented in the component that owns the state, as recommended.
156-167: Add validation for time range as suggested.The validation is not implemented anywhere in the codebase, and the suggested implementation is appropriate. The component already uses toast for error messaging, making the suggested approach consistent with the existing patterns.
apps/dashboard/components/virtual-table/hooks/useVirtualData.ts (3)
6-20: LGTM! Parameter changes align with v2 data fetching.The shift from passing the entire data array to just the total length improves performance by reducing prop drilling and memory usage.
21-38: LGTM! Well-implemented throttling mechanism.The throttling implementation is optimized with:
- Memoization to prevent unnecessary recreations
- Leading edge execution for responsive load more
- Proper cleanup to prevent memory leaks
74-81: LGTM! Well-configured virtualizer.The configuration is optimized with:
- Proper count handling for loading state
- Memoized callbacks
- Visual spacing with gap property
apps/dashboard/components/virtual-table/index.tsx (2)
Line range hint
16-39: LGTM! Well-structured data management.The separation of realtime and historic data with a unified table data hook provides a clean abstraction for managing different data sources.
Line range hint
150-170: LGTM! Well-structured row rendering.The row rendering logic is clean and handles all necessary states correctly. Consider updating the type assertions once the discriminated union type is implemented.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/dashboard/app/(app)/logs/components/table/logs-table.tsx (1)
120-125: 🛠️ Refactor suggestionAdd error handling and make polling interval configurable.
The real-time polling implementation needs error handling and configuration improvements.
Consider these improvements:
+const POLL_INTERVAL_MS = 2000; +const MAX_RETRIES = 3; +const BACKOFF_MS = 1000; export const LogsTable = () => { const { displayProperties, setSelectedLog, selectedLog, isLive } = useLogsContext(); - const { realtimeLogs, historicalLogs, isLoading, isLoadingMore, loadMore } = useLogsQuery({ + const { realtimeLogs, historicalLogs, isLoading, isLoadingMore, loadMore, error } = useLogsQuery({ startPolling: isLive, - pollIntervalMs: 2000, + pollIntervalMs: POLL_INTERVAL_MS, + maxRetries: MAX_RETRIES, + backoffMs: BACKOFF_MS, }); + if (error) { + return ( + <div className="w-full flex justify-center items-center h-full"> + <Empty className="w-[400px] flex items-start"> + <Empty.Icon className="w-auto text-error-9" /> + <Empty.Title>Error loading logs</Empty.Title> + <Empty.Description className="text-left"> + {error.message} + <button + onClick={() => loadMore()} + className="ml-2 text-accent-11 hover:underline" + > + Retry + </button> + </Empty.Description> + </Empty> + </div> + ); + }
🧹 Nitpick comments (1)
apps/dashboard/app/(app)/logs/components/table/logs-table.tsx (1)
100-118: Add null checks and extract header text transformation.The column generation logic could be improved for better maintainability and robustness.
Consider these improvements:
+const toTitleCase = (key: string) => + key.split('_') + .map(word => word.charAt(0).toUpperCase() + word.slice(1)) + .join(' '); const additionalColumns: Column<Log>[] = [ "response_body", "request_body", // ... ].map((key) => ({ key, - header: key - .split("_") - .map((word) => word.charAt(0).toUpperCase() + word.slice(1)) - .join(" "), + header: toTitleCase(key), width: "1fr", render: (log: Log) => ( <div className="font-mono whitespace-nowrap truncate"> - {log[key as keyof Log]} + {log[key as keyof Log] ?? '-'} </div> ), }));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/app/(app)/logs/components/table/logs-table.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (1)
apps/dashboard/app/(app)/logs/components/table/logs-table.tsx (1)
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2143
File: apps/dashboard/app/(app)/logs/logs-page.tsx:77-83
Timestamp: 2024-12-03T14:17:08.016Z
Learning: The `<LogsTable />` component already implements virtualization to handle large datasets efficiently.
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Test GO API Local / test_agent_local
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
apps/dashboard/app/(app)/logs/components/table/logs-table.tsx (4)
16-63: LGTM! Well-structured type definitions and styling constants.The styling system is well-organized, using semantic color tokens and covering all necessary states.
65-98: LGTM! Clean and focused utility functions.The utility functions follow good practices:
- Pure functions with clear responsibilities
- Correct HTTP status code ranges
- Consistent styling patterns
127-148: LGTM! Well-structured row styling logic.The complex row styling logic is well-organized and handles all edge cases appropriately:
- Real-time log highlighting
- Selected state emphasis
- Status-based styling
150-264: LGTM! Clean column definitions and informative empty state.The implementation shows attention to detail:
- Proper memoization of columns
- Clear empty state messaging
- Efficient warning icon integration
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/dashboard/app/(app)/logs/components/table/logs-table.tsx (1)
122-125: Make polling interval configurable.The polling interval is hardcoded to 2000ms. Consider making it configurable through environment variables or component props to allow for different requirements across environments.
+const DEFAULT_POLLING_INTERVAL = 2000; + export const LogsTable = () => { const { displayProperties, setSelectedLog, selectedLog, isLive } = useLogsContext(); const { realtimeLogs, historicalLogs, isLoading, isLoadingMore, loadMore } = useLogsQuery({ startPolling: isLive, - pollIntervalMs: 2000, + pollIntervalMs: process.env.NEXT_PUBLIC_LOGS_POLLING_INTERVAL + ? parseInt(process.env.NEXT_PUBLIC_LOGS_POLLING_INTERVAL, 10) + : DEFAULT_POLLING_INTERVAL, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/app/(app)/logs/components/table/logs-table.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (1)
apps/dashboard/app/(app)/logs/components/table/logs-table.tsx (1)
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2143
File: apps/dashboard/app/(app)/logs/logs-page.tsx:77-83
Timestamp: 2024-12-03T14:17:08.016Z
Learning: The `<LogsTable />` component already implements virtualization to handle large datasets efficiently.
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
apps/dashboard/app/(app)/logs/components/table/logs-table.tsx (3)
16-63: LGTM! Well-structured type definitions and styling constants.The type definitions and styling constants are well-organized, using semantic color tokens and covering all necessary states.
65-98: LGTM! Clean and well-implemented utility functions.The utility functions are pure, have clear responsibilities, and handle status codes correctly.
122-125: Consider implementing error boundaries and retry logic.The real-time polling implementation could benefit from error boundaries and retry logic to handle network failures gracefully.
… into feat/logs-v2-data-fetching
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/dashboard/app/(app)/logs/components/table/logs-table.tsx (3)
16-63: Consider enhancing type safety for HTTP status codes.The status code handling could benefit from stronger typing to prevent potential edge cases.
+type HttpStatus = 200 | 201 | 400 | 401 | 403 | 404 | 429 | 500 | 502 | 503 | 504; + type StatusStyle = { base: string; hover: string; selected: string; badge: { default: string; selected: string; }; focusRing: string; }; -const getStatusStyle = (status: number): StatusStyle => { +const getStatusStyle = (status: HttpStatus): StatusStyle => {
100-118: Consider memoizing additionalColumns and improving type safety.The column definitions could be optimized for performance and type safety.
-const additionalColumns: Column<Log>[] = [ +const ADDITIONAL_COLUMN_KEYS = [ "response_body", "request_body", "request_headers", "response_headers", "request_id", "workspace_id", "host", -].map((key) => ({ +] as const; + +type AdditionalColumnKey = typeof ADDITIONAL_COLUMN_KEYS[number]; + +const additionalColumns = useMemo(() => + ADDITIONAL_COLUMN_KEYS.map((key: AdditionalColumnKey) => ({ key, header: key .split("_") .map((word) => word.charAt(0).toUpperCase() + word.slice(1)) .join(" "), width: "1fr", render: (log: Log) => ( <div className="font-mono whitespace-nowrap truncate">{log[key]}</div> ), -})); +})), []);
149-150: Document the reason for ignoring exhaustive dependencies.The biome-ignore comment should include a more detailed explanation of why the exhaustive dependencies check is being ignored.
- // biome-ignore lint/correctness/useExhaustiveDependencies: it's okay + // biome-ignore lint/correctness/useExhaustiveDependencies: selectedLog.request_id is the only dependency needed for re-rendering the columns
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/app/(app)/logs/components/table/logs-table.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (1)
apps/dashboard/app/(app)/logs/components/table/logs-table.tsx (1)
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2143
File: apps/dashboard/app/(app)/logs/logs-page.tsx:77-83
Timestamp: 2024-12-03T14:17:08.016Z
Learning: The `<LogsTable />` component already implements virtualization to handle large datasets efficiently.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: autofix
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
apps/dashboard/app/(app)/logs/components/table/logs-table.tsx (3)
81-98: LGTM! Clean utility implementation.The utility functions are well-implemented with clear separation of concerns and good use of composition.
241-265: LGTM! Well-implemented virtual table with real-time updates.The implementation effectively handles:
- Real-time log updates with proper UI feedback
- Virtual scrolling for performance
- Clear empty state messaging
- Proper row selection and styling
120-126: 🛠️ Refactor suggestionConsider implementing error handling for failed queries.
The real-time polling implementation could benefit from error handling to improve user experience.
export const LogsTable = () => { const { displayProperties, setSelectedLog, selectedLog, isLive } = useLogsContext(); - const { realtimeLogs, historicalLogs, isLoading, isLoadingMore, loadMore } = useLogsQuery({ + const { realtimeLogs, historicalLogs, isLoading, isLoadingMore, loadMore, error } = useLogsQuery({ startPolling: isLive, pollIntervalMs: 2000, }); + + if (error) { + return ( + <div className="w-full flex justify-center items-center h-full"> + <Empty className="w-[400px]"> + <Empty.Icon /> + <Empty.Title>Error loading logs</Empty.Title> + <Empty.Description>{error.message}</Empty.Description> + <button + onClick={() => loadMore()} + className="mt-4 px-4 py-2 bg-accent-4 text-accent-11 rounded-md hover:bg-accent-5" + > + Retry + </button> + </Empty> + </div> + ); + }Likely invalid or redundant comment.
3 participants
What does this PR do?
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
Release Notes
New Features
sincefilter.DatetimePopoverandLogsLiveSwitch.LogsChartErrorandLogsChartLoadingcomponents for better error handling in charts.LogsRefreshcomponent for refreshing log data.useFetchTimeserieshook for fetching and formatting timeseries data.LogsDateTimecomponent for enhanced datetime filtering.Improvements
useLogsQueryhook for better state management and real-time updates.Bug Fixes