feat: logs page v2 #2701
feat: logs page v2 #2701
Conversation
Add virtual list to table to prevent rendering lots of items at once. Since we store items in the memory as we fetch more item, list might get big enough to slow the page down, thats why we have to partially show them.
Add debounce to combobox filters and add shortcut del/backspace to remove combobox filters when focused
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (11)
apps/dashboard/styles/tailwind/base.css (2)
14-19: LGTM! Consider adding color documentation.The chart color variables are well-structured using HSL format. The semantic naming and color choices provide good visual distinction for data visualization.
Consider adding a comment block above these variables documenting their intended usage in charts (e.g., which chart types or data series should use which colors).
136-150: Consider using CSS variables for color values.The code block styling implementation is clean, but the line number color is hardcoded using rgba. Consider using a CSS variable for better theme consistency.
code .line::before { content: counter(step); counter-increment: step; width: 1rem; margin-right: 1.5rem; display: inline-block; text-align: right; - color: rgba(115, 138, 148, 0.4); + color: var(--content-subtle); }apps/dashboard/app/(app)/logs/query-state.ts (3)
13-13: Consider reordering status codes logically.The status codes array mixes success (200) and error codes (400, 500). Consider reordering them for better readability:
-export const STATUSES = [400, 500, 200] as const; +export const STATUSES = [200, 400, 500] as const;
12-12: Consider exporting TIMELINE_OPTIONS constant.Since TIMELINE_OPTIONS is used in the Timeline type definition, it should be exported for consistency and potential reuse.
-const TIMELINE_OPTIONS = ["1h", "3h", "6h", "12h", "24h"] as const; +export const TIMELINE_OPTIONS = ["1h", "3h", "6h", "12h", "24h"] as const;
17-25: Add JSDoc comments for better type documentation.Consider adding JSDoc comments to document the purpose and constraints of each field in the QuerySearchParams type.
+/** + * Search parameters for filtering logs + */ export type QuerySearchParams = { + /** Host to filter logs by */ host: string; + /** Specific request ID to search for */ requestId: string; + /** HTTP method to filter by */ method: string; + /** URL path pattern to match */ path: string; + /** Array of HTTP status codes to filter by */ responseStatuses: ResponseStatus[]; + /** Start timestamp in milliseconds */ startTime: number; + /** Optional end timestamp in milliseconds */ endTime?: number; };apps/dashboard/app/(app)/logs/components/filters/components/response-status.tsx (1)
14-18: Consider expanding HTTP status code coverageThe current implementation has several limitations:
- Missing 3XX (Redirection) status codes
- Using string IDs that are later converted to numbers
- Oversimplified grouping of status codes
Consider this improved implementation:
const checkboxItems = [ - { id: "500", label: "Error", description: "5XX error codes" }, - { id: "200", label: "Success", description: "2XX success codes" }, - { id: "400", label: "Warning", description: "4XX warning codes" }, + { id: 500, label: "Server Error", description: "5XX server error codes" }, + { id: 200, label: "Success", description: "2XX success codes" }, + { id: 400, label: "Client Error", description: "4XX client error codes" }, + { id: 300, label: "Redirection", description: "3XX redirection codes" }, ];apps/dashboard/app/(app)/logs/page.tsx (1)
28-30: Enhance the error message for better user experienceConsider providing a more user-friendly error message when workspace is not found.
- return <div>Workspace with tenantId: {tenantId} not found</div>; + return <div className="text-center p-4"> + <h2 className="text-lg font-semibold">Workspace Not Found</h2> + <p className="text-gray-600">The requested workspace could not be found. Please check your access permissions.</p> + </div>;apps/dashboard/app/(app)/logs/components/logs-table-loading-row.tsx (1)
1-16: Consider extracting grid layout configurationThe grid column sizes are currently hardcoded and duplicated from the parent table. Consider extracting these values into shared constants to maintain consistency and prevent synchronization issues.
+const GRID_LAYOUT = "grid-cols-[166px_72px_20%_1fr]"; + export const LoadingRow = () => ( - <div className="absolute w-full font-mono grid grid-cols-[166px_72px_20%_1fr] text-[13px] leading-[14px] mb-[1px] h-[26px]"> + <div className={`absolute w-full font-mono grid ${GRID_LAYOUT} text-[13px] leading-[14px] mb-[1px] h-[26px]`}>apps/dashboard/app/(app)/logs/components/logs-table.tsx (3)
12-14: Consider moving constants to a shared configuration fileThese table-related constants might be needed by other components (like LoadingRow). Consider moving them to a shared configuration file to maintain consistency across components.
27-32: Consider optimizing virtualizer configurationThe current configuration works but could be enhanced:
- Consider adjusting overscan based on viewport size for better performance
- Memoize the estimateSize callback to prevent unnecessary recalculations
+const estimateSize = () => ROW_HEIGHT; + const virtualizer = useVirtualizer({ count: isLoading ? SKELETON_ROWS : logs?.length ?? 0, getScrollElement: () => parentRef.current, - estimateSize: () => ROW_HEIGHT, + estimateSize, - overscan: 5, + overscan: Math.ceil(window.innerHeight / ROW_HEIGHT), });
66-182: Optimize rendering performanceConsider the following performance optimizations:
- Memoize complex className calculations
- Extract inline styles to constants
- Use CSS Grid template areas for layout
+const VIRTUAL_ITEM_STYLE = (start: number) => ({ + position: 'absolute', + top: `${start}px`, + width: '100%', +}); + +const useLogStyles = (log: Log, selectedLog: Log | null) => { + return useMemo(() => ({ + className: cn( + "font-mono grid grid-cols-[166px_72px_20%_1fr]", + "text-[13px] leading-[14px] mb-[1px] rounded-[5px]", + "h-[26px] cursor-pointer absolute top-0 left-0 w-full", + "hover:bg-background-subtle/90 pl-1", + STATUS_STYLES[getStatusClass(log.response_status)], + selectedLog && { + "opacity-50": selectedLog.request_id !== log.request_id, + "opacity-100": selectedLog.request_id === log.request_id, + } + ), + style: VIRTUAL_ITEM_STYLE(virtualRow.start), + }), [log, selectedLog]); +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
apps/dashboard/app/(app)/logs/components/chart.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/filters/components/response-status.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/log-details/components/log-footer.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/logs-table-loading-row.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/logs-table.tsx(1 hunks)apps/dashboard/app/(app)/logs/logs-page.tsx(1 hunks)apps/dashboard/app/(app)/logs/page.tsx(1 hunks)apps/dashboard/app/(app)/logs/query-state.ts(1 hunks)apps/dashboard/components/timestamp-info.tsx(1 hunks)apps/dashboard/styles/tailwind/base.css(3 hunks)apps/dashboard/styles/tailwind/typography.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/dashboard/app/(app)/logs/components/log-details/components/log-footer.tsx
- apps/dashboard/app/(app)/logs/logs-page.tsx
- apps/dashboard/components/timestamp-info.tsx
- apps/dashboard/app/(app)/logs/components/chart.tsx
🧰 Additional context used
📓 Learnings (1)
apps/dashboard/app/(app)/logs/components/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.
🔇 Additional comments (14)
apps/dashboard/styles/tailwind/base.css (1)
66-71: LGTM! Dark mode colors are well-balanced.
The dark mode chart colors maintain good visibility while preserving the semantic relationship with their light mode counterparts.
apps/dashboard/styles/tailwind/typography.css (1)
149-149: LGTM! Backtick is more appropriate for code snippets.
The change from single quote to backtick is more semantically correct for code snippets.
apps/dashboard/app/(app)/logs/query-state.ts (3)
33-34: Consider timezone handling for date ranges.
Using Date.now() directly might lead to timezone-related issues. Consider using a more robust date handling approach.
37-41: Add time range validation.
The current implementation doesn't validate if startTime is before endTime.
10-10: Verify usage of PickKeys type.
The PickKeys utility type appears to be unused in the current file.
apps/dashboard/app/(app)/logs/components/filters/components/response-status.tsx (4)
1-13: LGTM! Clean imports and well-defined interface.
The imports are appropriate and the CheckboxItemProps interface is well-typed with clear property names.
25-35: Fix state update race condition in handleItemChange
The function uses stale state when updating search params.
50-69: Enhance accessibility for the filter popover
The popover needs proper ARIA attributes and keyboard navigation.
73-86: LGTM! Well-structured checkbox component.
The CheckboxItem component follows good practices:
- Proper HTML semantics with labels
- Good accessibility support
- Clear visual hierarchy
apps/dashboard/app/(app)/logs/page.tsx (5)
1-12: LGTM: Clean initialization with proper imports
The imports are well-organized and the searchParamsCache initialization follows the nuqs library pattern correctly.
14-19: Remove unused params type annotation
The params property is defined in the type annotation but not used in the function.
48-50: Avoid exposing internal error details to users
The error message still includes internal details that should not be exposed to users.
52-52: LGTM: Clean component rendering
The LogsPage component is rendered with appropriate props for initial data and workspace identification.
36-46: Verify log fetching parameters handling
The log fetching includes multiple optional parameters. Let's verify the parameter handling in the clickhouse API.
apps/dashboard/app/(app)/logs/components/filters/components/response-status.tsx
Show resolved
Hide resolved
|
|
|
|
There was a problem hiding this comment.
We should not parse the body into key-values, and more importantly we should't uppercase their keys, it loses the information that this was actually json, and while we always expect json, the user might mess up and send us something invalid.
I'm good with optimistically calling JSON.stringify(JSON.parse(body), null, 2) in a try catch to try and format it nicely but falling back to displaying the raw string
I agree, yeah. Let's do this. |
Those are actually padded from left on purpose, but we can change that if we are not happy. |
It looks a bit off and unintentional, but maybe that's just me let's keep it as is for now to move quickly |
This PR also adds escape listener when you navigating table with keyboard
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
apps/dashboard/app/(app)/logs/components/log-details/components/log-section.tsx (2)
3-9: Extract props interface for better maintainability.Consider extracting the props interface to improve code organization and reusability.
+interface LogSectionProps { + details: string | string[]; + title: string; +} + -export const LogSection = ({ - details, - title, -}: { - details: string | string[]; - title: string; -}) => { +export const LogSection = ({ details, title }: LogSectionProps) => {
11-14: Consider using design tokens for measurements.Replace hardcoded pixel values with design system tokens for better maintainability and consistency.
-<div className="px-3 flex flex-col gap-[2px]"> - <span className="text-sm text-content/65 font-sans">{title}</span> - <Card className="rounded-[5px] bg-background-subtle"> - <CardContent className="p-2 text-[12px]"> +<div className="px-3 flex flex-col gap-1"> + <span className="text-sm text-content/65 font-sans">{title}</span> + <Card className="rounded-md bg-background-subtle"> + <CardContent className="p-2 text-xs">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
apps/dashboard/app/(app)/logs/components/log-details/components/log-footer.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/log-details/components/log-meta.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/log-details/components/log-section.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/log-details/index.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/logs-table.tsx(1 hunks)apps/dashboard/app/(app)/logs/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/dashboard/app/(app)/logs/components/log-details/components/log-meta.tsx
- apps/dashboard/app/(app)/logs/components/log-details/index.tsx
- apps/dashboard/app/(app)/logs/components/logs-table.tsx
- apps/dashboard/app/(app)/logs/components/log-details/components/log-footer.tsx
🔇 Additional comments (4)
apps/dashboard/app/(app)/logs/components/log-details/components/log-section.tsx (1)
18-19:
Improve header parsing logic.
The current splitting logic might break for headers containing multiple colons.
-const [key, ...valueParts] = header.split(":");
-const value = valueParts.join(":").trim();
+const colonIndex = header.indexOf(':');
+const key = colonIndex > -1 ? header.slice(0, colonIndex) : header;
+const value = colonIndex > -1 ? header.slice(colonIndex + 1).trim() : '';apps/dashboard/app/(app)/logs/utils.ts (3)
1-11: LGTM! Well-structured error handling implementation.
The custom error class follows TypeScript best practices with proper type safety and immutable context.
13-48:
Add input validation and structured logging
Several security and robustness improvements are needed:
export const extractResponseField = <K extends keyof ResponseBody>(
log: Log,
fieldName: K,
): ResponseBody[K] | null => {
+ // Validate fieldName
+ if (typeof fieldName !== 'string' && typeof fieldName !== 'number' && typeof fieldName !== 'symbol') {
+ console.error('[extractResponseField] Invalid fieldName type', { fieldName });
+ return null;
+ }
+
if (!log?.response_body) {
- console.error("Invalid log or missing response_body");
+ console.error("[extractResponseField] Invalid log or missing response_body", { logId: log?.request_id });
return null;
}
+ // Validate response body size
+ const MAX_RESPONSE_BODY_SIZE = 1024 * 1024; // 1MB
+ if (log.response_body.length > MAX_RESPONSE_BODY_SIZE) {
+ console.error("[extractResponseField] Response body exceeds size limit", {
+ logId: log.request_id,
+ size: log.response_body.length
+ });
+ return null;
+ }
try {
const parsedBody = JSON.parse(log.response_body) as ResponseBody;1-84: Verify usage patterns and error handling consistency
Let's verify how these utilities are used across the codebase to ensure consistent error handling patterns.
✅ Verification successful
Consistent error handling and usage patterns verified
Based on the search results, I can confirm:
-
The utility functions are used consistently across the logs feature:
extractResponseFieldis used in log details components for parsing meta, code, and permissionsgetRequestHeaderis used for extracting user-agent informationsafeParseJsonis used for safely displaying request and response bodies
-
Error handling patterns are consistent:
- All functions properly handle edge cases with appropriate error logging
- Error messages are descriptive and include relevant context
- Console logging follows a consistent pattern across the codebase
-
The implementation aligns with the codebase's error handling practices:
- Uses
console.errorfor critical failures - Uses
console.warnfor non-critical issues - Includes contextual information in error messages
- Uses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage patterns of the utility functions
# Check for usage of extractResponseField
echo "Checking extractResponseField usage:"
rg "extractResponseField" -A 2
# Check for usage of getRequestHeader
echo "Checking getRequestHeader usage:"
rg "getRequestHeader" -A 2
# Check for usage of safeParseJson
echo "Checking safeParseJson usage:"
rg "safeParseJson" -A 2
# Check for consistent error logging patterns
echo "Checking error logging patterns:"
rg "console\.(error|warn)" --type ts
Length of output: 15622
apps/dashboard/app/(app)/logs/components/log-details/components/log-section.tsx
Show resolved
Hide resolved
apps/dashboard/app/(app)/logs/components/log-details/components/log-section.tsx
Show resolved
Hide resolved
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
LogsChartcomponent for visualizing log data with a bar chart.DatePickerWithRangecomponent for selecting custom date ranges.ResponseStatusfilter component for managing HTTP response status filters.SearchComboboxfor enhanced log search capabilities.Timelinecomponent for selecting time ranges for log filtering.LogsFilterscomponent to streamline log entry filtering.LogFooter,LogHeader,LogMetaSection, andLogSectioncomponents for detailed log entry views.LoadingRowcomponent to display loading states in the logs table.ResizablePanelfor improved log detail views.TimestampInfocomponent for enhanced timestamp displays.Calendarcomponent for date selection integrated with the UI.ButtonGroupcomponent for grouping buttons in the UI.Improvements
LogsTablefor better performance with large datasets.Bug Fixes
Documentation