feat: New logs page #2143
feat: New logs page #2143
Conversation
|
|
@ogzhanolguncu is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several new components and enhancements to the dashboard's logging functionality. Key changes include a new layout structure in Changes
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! 🙏 |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| function aggregateData(data: Log[]) { | ||
| const aggregatedData: { | ||
| date: string; | ||
| success: number; | ||
| warning: number; | ||
| error: number; | ||
| }[] = []; | ||
| const intervalMs = 60 * 1000 * 10; | ||
|
|
||
| if (data.length === 0) { | ||
| return aggregatedData; | ||
| } | ||
|
|
||
| const startOfDay = new Date(data[0].time).setHours(0, 0, 0, 0); | ||
| const endOfDay = startOfDay + 24 * 60 * 60 * 1000; | ||
|
|
||
| for (let timestamp = startOfDay; timestamp < endOfDay; timestamp += intervalMs) { | ||
| const filteredLogs = data.filter((d) => d.time >= timestamp && d.time < timestamp + intervalMs); | ||
|
|
||
| const success = filteredLogs.filter( | ||
| (log) => log.response_status >= 200 && log.response_status < 300, | ||
| ).length; | ||
| const warning = filteredLogs.filter( | ||
| (log) => log.response_status >= 400 && log.response_status < 500, | ||
| ).length; | ||
| const error = filteredLogs.filter((log) => log.response_status >= 500).length; | ||
|
|
||
| aggregatedData.push({ | ||
| date: format(timestamp, "yyyy-MM-dd'T'HH:mm:ss"), | ||
| success, | ||
| warning, | ||
| error, | ||
| }); | ||
| } | ||
|
|
||
| return aggregatedData; | ||
| } |
There was a problem hiding this comment.
yeah that feels a bit better, our arrays aren't huge, but as we might add more status types, we could end up looping over them quite a few times.
| }); | ||
| }} | ||
| /> | ||
| <CartesianGrid strokeDasharray="2" stroke="#DFE2E6" vertical={false} /> |
There was a problem hiding this comment.
how will this work in dark/light mode?
can we reference a css variable instead so it works automatically?
| to: new Date(), | ||
| }); | ||
| const [finalDate, setFinalDate] = useState<DateRange>(); | ||
| const [startTime, setStartTime] = useState({ HH: "09", mm: "00", ss: "00" }); |
There was a problem hiding this comment.
where do these defaults come from?
There was a problem hiding this comment.
It's just a random starting hour. Do you have something else on your mind?
apps/dashboard/app/(app)/logs/components/filters/components/response-status.tsx
Outdated
Show resolved
Hide resolved
apps/dashboard/app/(app)/logs/components/log-details/components/log-footer.tsx
Outdated
Show resolved
Hide resolved
apps/dashboard/app/(app)/logs/components/log-details/components/log-header.tsx
Outdated
Show resolved
Hide resolved
apps/dashboard/app/(app)/logs/components/log-details/components/meta-content.tsx
Show resolved
Hide resolved
![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 (16)
apps/dashboard/app/(app)/logs/page.tsx (1)
13-18: Remove unusedparamsfrom type definitionThe type definition includes
params: { slug: string }but this parameter isn't used in the component.export default async function Page({ searchParams, }: { - params: { slug: string }; searchParams: Record<string, string | string[] | undefined>; }) {apps/dashboard/components/ui/input.tsx (2)
4-9: LGTM! Consider adding JSDoc comments.The type definitions and imports are well-structured. The change from type to interface is a good choice for extensibility.
Consider adding JSDoc comments to document the props:
+/** + * Props for the Input component + * @property {LucideIcon} [startIcon] - Icon to display at the start of the input + * @property {LucideIcon} [endIcon] - Icon to display at the end of the input + */ export interface InputProps extends React.InputHTMLAttributes<HTMLInputElement> { startIcon?: LucideIcon; endIcon?: LucideIcon; }
23-33: Consider enhancing accessibility with aria-label.When icons are present, it would be helpful to provide additional context to screen readers.
<input type={type} className={cn( "flex h-8 w-full rounded-md border border-border bg-background px-3 py-2 text-sm focus:border-primary placeholder:text-content-subtle focus-visible:outline-none disabled:cursor-not-allowed disabled:opacity-50", startIcon ? "pl-8" : "", endIcon ? "pr-8" : "", className, )} ref={ref} + aria-label={startIcon || endIcon ? props['aria-label'] || props.placeholder : undefined} {...props} />apps/dashboard/app/(app)/logs/components/filters/components/time-split.tsx (2)
7-25: Consider making props interface readonly for better immutabilityThe type definitions are well-structured, but the interface could be improved by making the properties readonly to prevent accidental mutations.
export interface TimeSplitInputProps { - time: Time; + readonly time: Time; - setTime: (x: Time) => void; + readonly setTime: (x: Time) => void; - type: "start" | "end"; + readonly type: "start" | "end"; - setStartTime: (x: Time) => void; + readonly setStartTime: (x: Time) => void; - setEndTime: (x: Time) => void; + readonly setEndTime: (x: Time) => void; - startTime: Time; + readonly startTime: Time; - endTime: Time; + readonly endTime: Time; - startDate: Date; + readonly startDate: Date; - endDate: Date; + readonly endDate: Date; }
156-188: Improve time validation with constants and declarative checksThe validation logic uses magic numbers and could be more maintainable.
+const TIME_LIMITS = { + HH: 23, + mm: 59, + ss: 59 +} as const; + +const isValidTimeValue = (value: string, type: TimeType): boolean => { + if (!value || value.length > 2) return false; + const numValue = Number(value); + return !isNaN(numValue) && numValue >= 0 && numValue <= TIME_LIMITS[type]; +}; + function handleOnChange(value: string, valueType: TimeType) { - const payload = { - HH: time.HH, - mm: time.mm, - ss: time.ss, - }; - if (value.length > 2) return; - - switch (valueType) { - case "HH": - if (value && Number(value) > 23) return; - break; - case "mm": - if (value && Number(value) > 59) return; - break; - case "ss": - if (value && Number(value) > 59) return; - break; - default: - break; - } - - payload[valueType] = value; - setTime({ ...payload }); + if (!isValidTimeValue(value, valueType)) return; + setTime({ ...time, [valueType]: value }); }apps/dashboard/app/(app)/logs/utils.ts (1)
50-71: Add null checks and improve header parsing efficiency.The implementation is good but could be enhanced with additional safety checks and performance improvements.
Consider these improvements:
export const getRequestHeader = (log: Log, headerName: string): string | null => { - if (!headerName.trim()) { + if (!headerName?.trim()) { console.error("Invalid header name provided"); return null; } if (!Array.isArray(log.request_headers)) { console.error("request_headers is not an array"); return null; } const lowerHeaderName = headerName.toLowerCase(); - const header = log.request_headers.find((h) => h.toLowerCase().startsWith(`${lowerHeaderName}:`)); + // Use for...of for better performance as it can break early + let header: string | undefined; + for (const h of log.request_headers) { + if (h.toLowerCase().startsWith(`${lowerHeaderName}:`)) { + header = h; + break; + } + } if (!header) { console.warn(`Header "${headerName}" not found in request headers`); return null; } - const [, value] = header.split(":", 2); - return value ? value.trim() : null; + // More robust header value extraction + const colonIndex = header.indexOf(':'); + if (colonIndex === -1) { + console.warn(`Invalid header format: "${header}"`); + return null; + } + const value = header.slice(colonIndex + 1); + return value.trim() || null; };apps/dashboard/app/(app)/logs/query-state.ts (3)
14-14: Consider reorganizing and expanding HTTP status codes.The status codes are in an unusual order (400, 500, 200). Consider:
- Organizing them in ascending order for better readability
- Including other common status codes (e.g., 201, 401, 403, 404)
-export const STATUSES = [400, 500, 200] as const; +export const STATUSES = [200, 201, 400, 401, 403, 404, 500] as const;
18-26: Consider adding documentation and more specific types.The
QuerySearchParamstype could benefit from:
- JSDoc documentation explaining the purpose of each field
- A more specific type for the
methodfield (e.g., HTTP methods)+/** Parameters for filtering and searching logs */ export type QuerySearchParams = { + /** The host/domain to filter logs by */ host: string; + /** Unique identifier for the request */ requestId: string; - method: string; + /** HTTP method */ + method: 'GET' | 'POST' | 'PUT' | 'DELETE' | 'PATCH' | 'OPTIONS' | 'HEAD'; + /** URL path pattern to filter by */ path: string; + /** Array of HTTP status codes to filter by */ responseStatuses: ResponseStatus[]; + /** Start timestamp in milliseconds */ startTime: number; + /** End timestamp in milliseconds */ endTime: number; };
38-53: Enhance error handling in the hook.While the time range validation is good, consider:
- Using a more robust error handling mechanism instead of just console.warn
- Adding validation for other parameters
export const useLogSearchParams = () => { const [searchParams, setSearchParams] = useQueryStates(queryParamsPayload); const validateAndSetSearchParams = useCallback( (params: Partial<typeof searchParams>) => { + const errors: string[] = []; + if (params.startTime && params.endTime && params.startTime > params.endTime) { - console.warn("Invalid time range: start time is after end time"); - return; + errors.push("Invalid time range: start time is after end time"); } - setSearchParams(params); + + if (params.method && !['GET', 'POST', 'PUT', 'DELETE', 'PATCH', 'OPTIONS', 'HEAD'].includes(params.method)) { + errors.push("Invalid HTTP method"); + } + + if (errors.length > 0) { + console.error("Validation errors:", errors); + return errors; + } + + setSearchParams(params); + return null; }, [setSearchParams], ); return { searchParams, setSearchParams: validateAndSetSearchParams }; };apps/dashboard/app/(app)/logs/components/log-details/components/log-body.tsx (1)
40-42: Add security considerations for dangerouslySetInnerHTMLWhile the HTML is generated from a trusted source (shiki), it's good practice to add a comment explaining the security implications.
Add a comment explaining why dangerouslySetInnerHTML is safe in this context:
dangerouslySetInnerHTML={{ + // Safe to use dangerouslySetInnerHTML as HTML is generated by shiki __html: innerHtml, }}apps/dashboard/app/(app)/logs/components/log-details/index.tsx (1)
89-93: Use more specific key for header itemsUsing the entire header string as a key could lead to duplicate keys if headers have the same content.
Apply this diff:
- <span key={header}> + <span key={`${key}-${header}`}> <span className="text-content/65 capitalize">{key}</span> <span className="text-content whitespace-pre-line">: {value}</span> </span>apps/dashboard/app/(app)/logs/components/log-details/components/log-footer.tsx (1)
63-76: Add aria-label to outcome badgeThe Badge component should have an aria-label for better accessibility.
Apply this diff:
<Badge + aria-label={`Outcome status: ${content}`} className={cn( { "text-amber-11 bg-amber-3 hover:bg-amber-3 font-medium": YELLOW_STATES.includes(contentCopy), "text-red-11 bg-red-3 hover:bg-red-3 font-medium": RED_STATES.includes(contentCopy), }, "uppercase", )} > {content} </Badge>apps/dashboard/app/(app)/logs/components/logs-table.tsx (4)
14-17: Consider strengthening the type definition.While the current implementation works, you could make the type more explicit by using a readonly array type to prevent accidental mutations.
-export const LogsTable = ({ logs }: { logs?: Log[] }) => { +export const LogsTable = ({ logs }: { logs?: readonly Log[] }) => {
18-25: Consider memoizing virtualization configuration.The virtualization config could be memoized to prevent unnecessary recalculations.
+const OVERSCAN = 5; + const virtualizer = useVirtualizer({ count: logs?.length ?? 0, getScrollElement: () => parentRef.current, - estimateSize: () => ROW_HEIGHT, - overscan: 5, + estimateSize: useCallback(() => ROW_HEIGHT, []), + overscan: OVERSCAN, });Don't forget to add the import:
import { useCallback } from 'react';
100-122: Simplify status-based styling logic.The current implementation has complex nested conditions for status-based styling. Consider extracting this logic into a utility function.
const getStatusStyles = (status: number, isSelected: boolean) => { if (status >= 500) { return isSelected ? 'bg-red-3' : 'bg-red-2 text-red-11 hover:bg-red-3'; } if (status >= 400) { return isSelected ? 'bg-amber-3' : 'bg-amber-2 text-amber-11 hover:bg-amber-3'; } return isSelected ? 'bg-background-subtle/90' : ''; };Then simplify the className assignment:
-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", - { - "bg-amber-2 text-amber-11 hover:bg-amber-3": - l.response_status >= 400 && l.response_status < 500, - "bg-red-2 text-red-11 hover:bg-red-3": l.response_status >= 500, - }, - selectedLog && { - "opacity-50": selectedLog.request_id !== l.request_id, - "opacity-100": selectedLog.request_id === l.request_id, - "bg-background-subtle/90": - selectedLog.request_id === l.request_id && - l.response_status >= 200 && - l.response_status < 300, - "bg-amber-3": - selectedLog.request_id === l.request_id && - l.response_status >= 400 && - l.response_status < 500, - "bg-red-3": - selectedLog.request_id === l.request_id && l.response_status >= 500, - }, -) +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", + getStatusStyles(l.response_status, selectedLog?.request_id === l.request_id), + selectedLog && { + "opacity-50": selectedLog.request_id !== l.request_id, + "opacity-100": selectedLog.request_id === l.request_id, + } +)}
153-155: Consider adding a tooltip for truncated text.The response body is truncated but doesn't provide a way to view the full text without selecting the row.
-<div className="px-[2px] flex items-center max-w-[800px]"> - <span className="truncate">{l.response_body}</span> +<div className="px-[2px] flex items-center max-w-[800px]" title={l.response_body}> + <span className="truncate">{l.response_body}</span> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
apps/dashboard/app/(app)/logs/components/filters/components/custom-date-filter.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/filters/components/time-split.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/filters/components/timeline.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/filters/index.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/log-details/components/log-body.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/log-details/components/log-footer.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/log-details/components/log-header.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/log-details/components/request-response-details.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/logs-page.tsx(1 hunks)apps/dashboard/app/(app)/logs/page.tsx(2 hunks)apps/dashboard/app/(app)/logs/query-state.ts(1 hunks)apps/dashboard/app/(app)/logs/utils.ts(1 hunks)apps/dashboard/components/ui/command.tsx(1 hunks)apps/dashboard/components/ui/group-button.tsx(1 hunks)apps/dashboard/components/ui/input.tsx(1 hunks)apps/dashboard/lib/trpc/routers/logs/query-log.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- apps/dashboard/app/(app)/logs/components/log-details/components/log-header.tsx
- apps/dashboard/app/(app)/logs/components/filters/components/timeline.tsx
- apps/dashboard/components/ui/command.tsx
- apps/dashboard/app/(app)/logs/components/filters/components/custom-date-filter.tsx
- apps/dashboard/app/(app)/logs/components/filters/index.tsx
- apps/dashboard/lib/trpc/routers/logs/query-log.ts
- apps/dashboard/app/(app)/logs/logs-page.tsx
- apps/dashboard/components/ui/group-button.tsx
- apps/dashboard/app/(app)/logs/components/log-details/components/request-response-details.tsx
🧰 Additional context used
📓 Learnings (2)
apps/dashboard/app/(app)/logs/components/log-details/components/log-footer.tsx (1)
Learnt from: chronark
PR: unkeyed/unkey#2143
File: apps/dashboard/app/(app)/logs/components/log-details/components/log-footer.tsx:58-61
Timestamp: 2024-12-03T14:21:19.543Z
Learning: For the "Outcome" field in the `LogFooter` component (`apps/dashboard/app/(app)/logs/components/log-details/components/log-footer.tsx`), default to "N/A" instead of "VALID" when handling null values to avoid confusing customers.
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 (17)
apps/dashboard/app/(app)/logs/page.tsx (5)
Line range hint 22-29: LGTM! Proper workspace validation
The workspace fetching includes proper null checks and tenant validation.
30-40: LGTM! Comprehensive log fetching parameters
The logs API call includes all necessary filtering parameters from the parsed search params.
41-43: LGTM! Error handling follows best practices
The error handling has been implemented correctly as suggested in the previous review.
7-7: Verify the default logs fetch count value
Let's ensure the default fetch count is appropriate for performance and UX.
✅ Verification successful
Default logs fetch count value is reasonable
The constant DEFAULT_LOGS_FETCH_COUNT is set to 100 entries, which is a reasonable default that balances between providing sufficient data for users and maintaining good performance. This value is appropriately used in the logs page component for initial fetching.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the DEFAULT_LOGS_FETCH_COUNT value
cat apps/dashboard/app/(app)/logs/constants.ts | grep DEFAULT_LOGS_FETCH_COUNT
Length of output: 261
Script:
#!/bin/bash
# Search for the constant using ripgrep to handle special characters in path
rg "DEFAULT_LOGS_FETCH_COUNT" -A 1
Length of output: 500
45-45: Verify LogsPage component props
Let's ensure the LogsPage component properly handles these props.
✅ Verification successful
Props are correctly handled by LogsPage component
The LogsPage component properly accepts and uses both initialLogs and workspaceId props:
initialLogsis used to initialize the logs stateworkspaceIdis used in the TRPC query for fetching logs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check LogsPage component implementation
ast-grep --pattern 'export function LogsPage({ initialLogs, workspaceId }: { $_ }) {
$$$
}'
Length of output: 95
Script:
#!/bin/bash
# Let's try a broader search for LogsPage component
ast-grep --pattern 'LogsPage'
# Also search for the file containing LogsPage
fd LogsPage
Length of output: 313
Script:
#!/bin/bash
# Let's check the implementation of LogsPage in logs-page.tsx
cat apps/dashboard/app/\(app\)/logs/logs-page.tsx
Length of output: 2469
apps/dashboard/components/ui/input.tsx (2)
12-33: LGTM! Clean and efficient implementation.
The component implementation is well-structured with proper:
- Props handling and ref forwarding
- Conditional rendering of icons
- Dynamic className management
- Performance considerations through icon component memoization
1-42: Verify integration with logs page components.
The Input component's design aligns well with the logs page requirements. However, ensure consistent usage across the logs page components (LogsChart, LogsFilters) for a unified experience.
✅ Verification successful
Let me check the actual usage patterns in these files to verify the integration.
Let me try again with the correct command.
Input component is properly integrated across logs page components
The verification shows consistent and proper usage of the Input component across the logs page:
- Used in search-combobox for search functionality with proper ref handling and keyboard interactions
- Integrated with appropriate props and styling in the time-split component for date/time filtering
- Maintains consistent styling and behavior through proper prop passing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage patterns of the Input component in logs page components
# Search for Input component usage in logs-related files
rg -l "Input" apps/dashboard/app/\(app\)/logs/
Length of output: 471
Script:
#!/bin/bash
# Check how Input component is used in the logs-related files
rg "Input" --type typescript --context 3 apps/dashboard/app/\(app\)/logs/components/
Length of output: 119
Script:
#!/bin/bash
# Check how Input component is used in the logs-related files
rg "Input" -A 3 -B 3 apps/dashboard/app/\(app\)/logs/components/filters/components/
Length of output: 17823
apps/dashboard/app/(app)/logs/components/filters/components/time-split.tsx (2)
1-6: LGTM! Good attribution and clean imports.
The code properly attributes the original source and includes only the necessary imports.
195-276: Previous review comments are still applicable
The following issues from previous reviews remain relevant:
- useEffect dependencies and lint ignore
- Input field implementation improvements
apps/dashboard/app/(app)/logs/utils.ts (2)
3-11: LGTM! Well-structured custom error class.
The ResponseBodyParseError class is well-implemented with proper typing and follows best practices for custom error classes in TypeScript.
13-48: Verify the correct ID field usage and add type safety improvements.
The implementation looks solid but has a few areas for improvement:
Consider these improvements:
- Add runtime type validation after the type assertion:
-const parsedBody = JSON.parse(log.response_body) as ResponseBody;
+const parsedBody = JSON.parse(log.response_body) as unknown;
+const isResponseBody = (obj: unknown): obj is ResponseBody => {
+ return obj !== null && typeof obj === 'object';
+};
+if (!isResponseBody(parsedBody)) {
+ throw new ResponseBodyParseError("Invalid response body structure", parsedBody);
+}- Add try-catch specifically for JSON.parse:
try {
- const parsedBody = JSON.parse(log.response_body) as ResponseBody;
+ let parsedBody;
+ try {
+ parsedBody = JSON.parse(log.response_body) as ResponseBody;
+ } catch (e) {
+ throw new ResponseBodyParseError("Invalid JSON in response_body", {
+ error: e instanceof Error ? e.message : "Unknown parsing error"
+ });
+ }apps/dashboard/app/(app)/logs/query-state.ts (2)
13-13: Export TIMELINE_OPTIONS if intended for external use.
The TIMELINE_OPTIONS constant is defined but not exported. If it's intended to be used by other components, consider exporting it.
11-11: Consider removing or utilizing the PickKeys type.
The PickKeys type appears to be unused within this file. If it's not being used elsewhere, consider removing it. If it's intended for external use, consider adding documentation explaining its purpose and usage.
✅ Verification successful
PickKeys type is correctly used and serves a specific purpose
The PickKeys type is actively used in the search-combobox component to create strongly-typed options for filtering logs. It ensures type safety by restricting the possible filter keys to specific fields of QuerySearchParams ("host", "requestId", "path", "method"). The type definition is appropriate and serves its purpose well.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of PickKeys type
rg "PickKeys" --type ts
Length of output: 467
Script:
#!/bin/bash
# Get the content of the file using the PickKeys type to understand its usage
cat apps/dashboard/app/\(app\)/logs/components/filters/components/search-combobox/hooks.ts
Length of output: 3244
apps/dashboard/app/(app)/logs/components/log-details/components/log-body.tsx (2)
12-15: 🛠️ Refactor suggestion
Move highlighter initialization outside component
The highlighter initialization should be moved outside the component to prevent recreation on each render.
Apply this diff:
-const highlighter = createHighlighter({
+const getHighlighter = (() => {
+ let instance: Promise<any> | null = null;
+ return () => {
+ if (!instance) {
+ instance = createHighlighter({
+ themes: ["github-light", "github-dark"],
+ langs: ["json"],
+ });
+ }
+ return instance;
+ };
+})();20-32:
Add error handling for JSON parsing and highlighting
The code lacks proper error handling for JSON parsing and highlighting operations, and doesn't clean up properly.
Apply this diff:
useEffect(() => {
+ let mounted = true;
+
+ try {
+ const parsedJson = JSON.parse(field);
highlighter.then((highlight) => {
- const html = highlight.codeToHtml(JSON.stringify(JSON.parse(field), null, 2), {
+ if (!mounted) return;
+ const html = highlight.codeToHtml(JSON.stringify(parsedJson, null, 2), {
lang: "json",
themes: {
dark: "github-dark",
light: "github-light",
},
mergeWhitespaces: true,
});
setHtml(html);
+ }).catch((error) => {
+ console.error('Failed to highlight code:', error);
+ setHtml('Failed to highlight code');
});
+ } catch (error) {
+ console.error('Invalid JSON:', error);
+ setHtml('Invalid JSON');
+ }
+
+ return () => {
+ mounted = false;
+ };
}, [field]);Likely invalid or redundant comment.
apps/dashboard/app/(app)/logs/components/logs-table.tsx (2)
1-13: LGTM! Well-organized imports and meaningful constants.
The imports are logically grouped and the constants provide clear styling parameters for the table.
69-99:
Improve keyboard navigation accessibility.
The current keyboard navigation implementation has several issues:
- Using
tabIndexon non-interactive elements - Direct DOM manipulation for focus management
- Missing ARIA roles and labels
-<div
+<button
key={virtualRow.key}
data-index={virtualRow.index}
ref={virtualizer.measureElement}
onClick={() => handleLogSelection(l)}
- tabIndex={virtualRow.index}
+ role="row"
+ aria-label={`Log entry for ${l.path} with status ${l.response_status}`}
aria-selected={selectedLog?.request_id === l.request_id}
onKeyDown={(event) => {
if (event.key === "Enter" || event.key === " ") {
event.preventDefault();
handleLogSelection(l);
}
- if (event.key === "ArrowDown") {
- event.preventDefault();
- const nextElement = document.querySelector(
- `[data-index="${virtualRow.index + 1}"]`,
- ) as HTMLElement;
- nextElement?.focus();
- }
- if (event.key === "ArrowUp") {
- event.preventDefault();
- const prevElement = document.querySelector(
- `[data-index="${virtualRow.index - 1}"]`,
- ) as HTMLElement;
- prevElement?.focus();
- }
}}Consider using a proper table structure with role="grid" and implementing keyboard navigation using a focus management library or React's ref system.
Likely invalid or redundant comment.
| } | ||
| if (!_time.mm) { | ||
| _time.mm = "00"; | ||
| } | ||
| if (!_time.ss) { | ||
| _time.ss = "00"; | ||
| } | ||
|
|
||
| let endTimeChanges = false; | ||
| const endTimePayload = endTime; | ||
|
|
||
| let startTimeChanges = false; | ||
| const startTimePayload = startTime; | ||
|
|
||
| // Only run time conflicts if | ||
| // startDate and endDate are the same date | ||
|
|
||
| if (format(new Date(startDate), "dd/mm/yyyy") === format(new Date(endDate), "dd/mm/yyyy")) { | ||
| // checks if start time is ahead of end time | ||
|
|
||
| if (type === "start") { | ||
| if (_time.HH && Number(_time.HH) > Number(endTime.HH)) { | ||
| endTimePayload.HH = _time.HH; | ||
| endTimeChanges = true; | ||
| } | ||
|
|
||
| if ( | ||
| // also check the hour | ||
| _time.HH && | ||
| Number(_time.HH) >= Number(endTime.HH) && | ||
| // check the minutes | ||
| _time.mm && | ||
| Number(_time.mm) > Number(endTime.mm) | ||
| ) { | ||
| endTimePayload.mm = _time.mm; | ||
| endTimeChanges = true; | ||
| } | ||
|
|
||
| if ( | ||
| // also check the hour | ||
| _time.HH && | ||
| Number(_time.HH) >= Number(endTime.HH) && | ||
| // check the minutes | ||
| _time.mm && | ||
| Number(_time.mm) >= Number(endTime.mm) && | ||
| // check the seconds | ||
| _time.ss && | ||
| Number(_time.ss) > Number(endTime.ss) | ||
| ) { | ||
| endTimePayload.ss = _time.ss; | ||
| endTimeChanges = true; | ||
| } | ||
| } | ||
|
|
||
| if (type === "end") { | ||
| if (_time.HH && Number(_time.HH) < Number(startTime.HH)) { | ||
| startTimePayload.HH = _time.HH; | ||
| startTimeChanges = true; | ||
| } | ||
|
|
||
| if ( | ||
| // also check the hour | ||
| _time.HH && | ||
| Number(_time.HH) <= Number(startTime.HH) && | ||
| // check the minutes | ||
| _time.mm && | ||
| Number(_time.mm) < Number(startTime.mm) | ||
| ) { | ||
| startTimePayload.mm = _time.mm; | ||
| startTimeChanges = true; | ||
| } | ||
|
|
||
| if ( | ||
| // also check the hour | ||
| _time.HH && | ||
| Number(_time.HH) <= Number(startTime.HH) && | ||
| // check the minutes | ||
| _time.mm && | ||
| Number(_time.mm) <= Number(startTime.mm) && | ||
| // check the seconds | ||
| _time.ss && | ||
| Number(_time.ss) < Number(startTime.ss) | ||
| ) { | ||
| startTimePayload.ss = _time.ss; | ||
| startTimeChanges = true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| setTime({ ..._time }); | ||
|
|
||
| if (endTimeChanges) { | ||
| setEndTime({ ...endTimePayload }); | ||
| } | ||
| if (startTimeChanges) { | ||
| setStartTime({ ...startTimePayload }); | ||
| } | ||
|
|
||
| setFocus(false); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve handleOnBlur implementation for better maintainability
The current implementation has several issues:
- Direct mutation of the time object
- Complex nested conditions for time conflict resolution
- Mixed responsibilities (formatting and conflict resolution)
Consider splitting the function into smaller, focused functions:
const formatTimeSegment = (value: string) => (!value ? "00" : value.padStart(2, "0"));
const formatTime = (time: Time): Time => ({
HH: formatTimeSegment(time.HH),
mm: formatTimeSegment(time.mm),
ss: formatTimeSegment(time.ss)
});
const isTimeConflict = (
type: "start" | "end",
currentTime: Time,
startTime: Time,
endTime: Time
): { startTimeChanges: boolean; endTimeChanges: boolean; updates: Partial<Time> } => {
// Extract conflict detection logic here
// Return necessary updates without mutations
};
function handleOnBlur() {
const formattedTime = formatTime(time);
if (format(startDate, "dd/mm/yyyy") === format(endDate, "dd/mm/yyyy")) {
const { startTimeChanges, endTimeChanges, updates } = isTimeConflict(
type,
formattedTime,
startTime,
endTime
);
setTime(formattedTime);
if (endTimeChanges) setEndTime({ ...endTime, ...updates });
if (startTimeChanges) setStartTime({ ...startTime, ...updates });
} else {
setTime(formattedTime);
}
setFocus(false);
}
apps/dashboard/app/(app)/logs/components/log-details/components/log-footer.tsx
Show resolved
Hide resolved
| const handleLogSelection = (log: Log) => { | ||
| setSelectedLog(log); | ||
| setTableDistanceToTop( | ||
| document.getElementById("log-table")!.getBoundingClientRect().top + | ||
| window.scrollY - | ||
| TABLE_BORDER_THICKNESS, | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Replace direct DOM manipulation with ref-based approach.
The current implementation uses direct DOM manipulation with getElementById and non-null assertion (!), which could lead to runtime errors if the element is not found.
+const tableRef = useRef<HTMLDivElement>(null);
+
const handleLogSelection = (log: Log) => {
setSelectedLog(log);
- setTableDistanceToTop(
- document.getElementById("log-table")!.getBoundingClientRect().top +
- window.scrollY -
- TABLE_BORDER_THICKNESS,
- );
+ if (tableRef.current) {
+ setTableDistanceToTop(
+ tableRef.current.getBoundingClientRect().top +
+ window.scrollY -
+ TABLE_BORDER_THICKNESS,
+ );
+ }
};Then update the div reference:
-<div className="h-[75vh] overflow-auto" id="log-table" ref={parentRef}>
+<div className="h-[75vh] overflow-auto" ref={(el) => {
+ parentRef.current = el;
+ tableRef.current = el;
+}}>Committable suggestion skipped: line range outside the PR's diff.
Improve performance of chart calculation. And, move meta field of log details out of log footer
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
apps/dashboard/app/(app)/logs/query-state.ts (1)
13-13: Consider exporting TIMELINE_OPTIONS constantThe constant is used to define the exported Timeline type but isn't exported itself. This could limit reusability if other components need access to the timeline options.
-const TIMELINE_OPTIONS = ["1h", "3h", "6h", "12h", "24h"] as const; +export const TIMELINE_OPTIONS = ["1h", "3h", "6h", "12h", "24h"] as const;apps/dashboard/app/(app)/logs/components/chart.tsx (1)
150-153: Add block statements for better readabilityFollowing the static analysis suggestion, wrap single-line if statements in blocks for consistency and maintainability.
- if (status >= 200 && status < 300) bucket.success++; - else if (status >= 400 && status < 500) bucket.warning++; - else if (status >= 500) bucket.error++; + if (status >= 200 && status < 300) { + bucket.success++; + } else if (status >= 400 && status < 500) { + bucket.warning++; + } else if (status >= 500) { + bucket.error++; + }🧰 Tools
🪛 Biome (1.9.4)
[error] 150-152: Block statements are preferred in this position.
Unsafe fix: Wrap the statement with a
JsBlockStatement(lint/style/useBlockStatements)
[error] 151-152: Block statements are preferred in this position.
Unsafe fix: Wrap the statement with a
JsBlockStatement(lint/style/useBlockStatements)
[error] 152-152: Block statements are preferred in this position.
Unsafe fix: Wrap the statement with a
JsBlockStatement(lint/style/useBlockStatements)
apps/dashboard/app/(app)/logs/components/log-details/components/log-meta.tsx (1)
3-14: Add prop validation and improve responsive designConsider these improvements:
- Add prop validation to ensure content is always a string
- Make the width responsive instead of fixed
-export const LogMetaSection = ({ content }: { content: string }) => { +import { type FC } from 'react'; + +interface LogMetaSectionProps { + content: string; +} + +export const LogMetaSection: FC<LogMetaSectionProps> = ({ content }) => { return ( <div className="flex justify-between pt-2.5 px-3"> <div className="text-sm text-content/65 font-sans">Meta</div> <Card className="rounded-[5px] flex"> - <CardContent className="text-[12px] w-[300px] flex-2 bg-background-subtle p-3"> + <CardContent className="text-[12px] w-full min-w-[300px] max-w-[500px] flex-2 bg-background-subtle p-3"> <pre>{content}</pre> </CardContent> </Card>apps/dashboard/app/(app)/logs/components/log-details/index.tsx (2)
22-28: Consider using ResizeObserver instead of debounceThe current debounce implementation might cause jank during resizing.
-const PANEL_WIDTH_SET_DELAY = 150; +import { useResizeObserver } from '@/hooks/use-resize-observer'; const _LogDetails = ({ log, onClose, distanceToTop }: Props) => { - const [panelWidth, setPanelWidth] = useState(DEFAULT_DRAGGABLE_WIDTH); - - const debouncedSetPanelWidth = useDebounceCallback((newWidth) => { - setPanelWidth(newWidth); - }, PANEL_WIDTH_SET_DELAY); + const { ref, width } = useResizeObserver<HTMLDivElement>(); + const panelWidth = width ?? DEFAULT_DRAGGABLE_WIDTH;
52-54: Remove unnecessary div and negative marginThe empty div with negative margin seems to be a hack.
<div className="space-y-3 border-b-[1px] border-border py-4"> - <div className="mt-[-24px]" /> <LogSection details={log.request_headers} title="Request Header" />Consider adjusting the padding of the parent container instead if more space is needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
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/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/query-state.ts(1 hunks)apps/dashboard/package.json(2 hunks)apps/dashboard/styles/tailwind/base.css(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/dashboard/package.json
- apps/dashboard/app/(app)/logs/components/log-details/components/log-footer.tsx
- apps/dashboard/app/(app)/logs/components/filters/components/response-status.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
apps/dashboard/app/(app)/logs/components/chart.tsx
[error] 150-152: Block statements are preferred in this position.
Unsafe fix: Wrap the statement with a JsBlockStatement
(lint/style/useBlockStatements)
[error] 151-152: Block statements are preferred in this position.
Unsafe fix: Wrap the statement with a JsBlockStatement
(lint/style/useBlockStatements)
[error] 152-152: Block statements are preferred in this position.
Unsafe fix: Wrap the statement with a JsBlockStatement
(lint/style/useBlockStatements)
🔇 Additional comments (6)
apps/dashboard/app/(app)/logs/query-state.ts (3)
18-26: LGTM! Well-structured type definition
The QuerySearchParams type clearly defines all necessary fields for the search parameters.
40-44: Consider adding error handling for invalid query parameters
The useLogSearchParams hook should handle cases where query parameters are invalid or malformed.
28-38:
Fix naming inconsistency in query parameters
The parameter name responseStatus in queryParamsPayload doesn't match the responseStatuses field in QuerySearchParams type.
- responseStatus: parseAsArrayOf(parseAsNumberLiteral(STATUSES)).withDefault([]),
+ responseStatuses: parseAsArrayOf(parseAsNumberLiteral(STATUSES)).withDefault([]),Likely invalid or redundant comment.
apps/dashboard/styles/tailwind/base.css (1)
14-19: LGTM! Well-implemented chart color system
The chart colors are properly defined with HSL values and have appropriate dark mode variants. The use of CSS variables allows for consistent theming across the application.
Also applies to: 66-71
apps/dashboard/app/(app)/logs/components/chart.tsx (1)
112-157: Consider optimizing aggregateData function
The current implementation could be optimized for performance with large datasets.
🧰 Tools
🪛 Biome (1.9.4)
[error] 150-152: Block statements are preferred in this position.
Unsafe fix: Wrap the statement with a JsBlockStatement
(lint/style/useBlockStatements)
[error] 151-152: Block statements are preferred in this position.
Unsafe fix: Wrap the statement with a JsBlockStatement
(lint/style/useBlockStatements)
[error] 152-152: Block statements are preferred in this position.
Unsafe fix: Wrap the statement with a JsBlockStatement
(lint/style/useBlockStatements)
apps/dashboard/app/(app)/logs/components/log-details/index.tsx (1)
56-57:
Add error handling for JSON parsing operations
The code assumes JSON.parse will always succeed and the result will be an object.
<LogSection
- details={flattenObject(JSON.parse(log.request_body))}
+ details={(() => {
+ try {
+ const parsed = JSON.parse(log.request_body);
+ if (typeof parsed !== 'object' || parsed === null) {
+ throw new Error('Expected object');
+ }
+ return flattenObject(parsed);
+ } catch (error) {
+ console.error('Invalid request body:', error);
+ return ['Error: Invalid JSON or non-object data'];
+ }
+ })()}
title="Request Body"
/>Apply the same pattern to the response body parsing.
Also applies to: 61-62
| error: 0, | ||
| }); | ||
| } | ||
| console.log(buckets); |
There was a problem hiding this comment.
Remove console.log statement
Remove debugging console.log statement before deploying to production.
- console.log(buckets);| <Bar | ||
| dataKey="success" | ||
| stackId="a" | ||
| fill="var(--color-success)" | ||
| radius={3} | ||
| /> | ||
| <Bar | ||
| dataKey="warning" | ||
| stackId="a" | ||
| fill="var(--color-warning)" | ||
| radius={3} | ||
| /> | ||
| <Bar dataKey="error" stackId="a" fill="var(--color-error)" radius={3} /> |
There was a problem hiding this comment.
Fix inconsistency in color variable usage
The Bar components use different color variables (--color-success) than those defined in chartConfig (--chart-3). This could lead to inconsistent styling.
- fill="var(--color-success)"
+ fill={chartConfig.success.color}
- fill="var(--color-warning)"
+ fill={chartConfig.warning.color}
- fill="var(--color-error)"
+ fill={chartConfig.error.color}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Bar | |
| dataKey="success" | |
| stackId="a" | |
| fill="var(--color-success)" | |
| radius={3} | |
| /> | |
| <Bar | |
| dataKey="warning" | |
| stackId="a" | |
| fill="var(--color-warning)" | |
| radius={3} | |
| /> | |
| <Bar dataKey="error" stackId="a" fill="var(--color-error)" radius={3} /> | |
| <Bar | |
| dataKey="success" | |
| stackId="a" | |
| fill={chartConfig.success.color} | |
| radius={3} | |
| /> | |
| <Bar | |
| dataKey="warning" | |
| stackId="a" | |
| fill={chartConfig.warning.color} | |
| radius={3} | |
| /> | |
| <Bar dataKey="error" stackId="a" fill={chartConfig.error.color} radius={3} /> |
| {details.map((header) => { | ||
| const [key, ...valueParts] = header.split(":"); | ||
| const value = valueParts.join(":").trim(); | ||
| return ( | ||
| <span key={header}> | ||
| <span className="text-content/65 capitalize">{key}</span> | ||
| <span className="text-content whitespace-pre-line"> | ||
| : {value} | ||
| </span> | ||
| {"\n"} | ||
| </span> | ||
| ); | ||
| })} |
There was a problem hiding this comment.
Add error handling for malformed detail strings
The current implementation assumes all detail strings contain a colon separator. Add error handling to gracefully handle malformed inputs.
{details.map((header) => {
+ if (!header.includes(':')) {
+ console.warn(`Malformed detail string: ${header}`);
+ return (
+ <span key={header}>
+ <span className="text-content/65">{header}</span>
+ {"\n"}
+ </span>
+ );
+ }
const [key, ...valueParts] = header.split(":");
const value = valueParts.join(":").trim();
return (
<span key={header}>
<span className="text-content/65 capitalize">{key}</span>
<span className="text-content whitespace-pre-line">
: {value}
</span>
{"\n"}
</span>
);
})}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {details.map((header) => { | |
| const [key, ...valueParts] = header.split(":"); | |
| const value = valueParts.join(":").trim(); | |
| return ( | |
| <span key={header}> | |
| <span className="text-content/65 capitalize">{key}</span> | |
| <span className="text-content whitespace-pre-line"> | |
| : {value} | |
| </span> | |
| {"\n"} | |
| </span> | |
| ); | |
| })} | |
| {details.map((header) => { | |
| if (!header.includes(':')) { | |
| console.warn(`Malformed detail string: ${header}`); | |
| return ( | |
| <span key={header}> | |
| <span className="text-content/65">{header}</span> | |
| {"\n"} | |
| </span> | |
| ); | |
| } | |
| const [key, ...valueParts] = header.split(":"); | |
| const value = valueParts.join(":").trim(); | |
| return ( | |
| <span key={header}> | |
| <span className="text-content/65 capitalize">{key}</span> | |
| <span className="text-content whitespace-pre-line"> | |
| : {value} | |
| </span> | |
| {"\n"} | |
| </span> | |
| ); | |
| })} |
| function flattenObject(obj: object, prefix = ""): string[] { | ||
| return Object.entries(obj).flatMap(([key, value]) => { | ||
| const newKey = prefix ? `${prefix}.${key}` : key; | ||
| if (typeof value === "object" && value !== null) { | ||
| return flattenObject(value, newKey); | ||
| } | ||
| return `${newKey}:${value}`; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety in flattenObject utility
The function needs better type safety and null checks.
-function flattenObject(obj: object, prefix = ""): string[] {
+function flattenObject(obj: Record<string, unknown>, prefix = ""): string[] {
+ if (typeof obj !== 'object' || obj === null) {
+ throw new TypeError('Expected non-null object');
+ }
return Object.entries(obj).flatMap(([key, value]) => {
const newKey = prefix ? `${prefix}.${key}` : key;
- if (typeof value === "object" && value !== null) {
- return flattenObject(value, newKey);
+ if (value === null) {
+ return `${newKey}:null`;
+ }
+ if (typeof value === "object") {
+ return flattenObject(value as Record<string, unknown>, newKey);
}
return `${newKey}:${value}`;
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function flattenObject(obj: object, prefix = ""): string[] { | |
| return Object.entries(obj).flatMap(([key, value]) => { | |
| const newKey = prefix ? `${prefix}.${key}` : key; | |
| if (typeof value === "object" && value !== null) { | |
| return flattenObject(value, newKey); | |
| } | |
| return `${newKey}:${value}`; | |
| }); | |
| } | |
| function flattenObject(obj: Record<string, unknown>, prefix = ""): string[] { | |
| if (typeof obj !== 'object' || obj === null) { | |
| throw new TypeError('Expected non-null object'); | |
| } | |
| return Object.entries(obj).flatMap(([key, value]) => { | |
| const newKey = prefix ? `${prefix}.${key}` : key; | |
| if (value === null) { | |
| return `${newKey}:null`; | |
| } | |
| if (typeof value === "object") { | |
| return flattenObject(value as Record<string, unknown>, newKey); | |
| } | |
| return `${newKey}:${value}`; | |
| }); | |
| } |
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
apps/dashboard/app/(app)/logs/query-state.ts (1)
13-13: Consider exporting TIMELINE_OPTIONS constantThe constant is used to define the exported Timeline type but isn't exported itself. This could limit reusability if other components need access to the timeline options.
-const TIMELINE_OPTIONS = ["1h", "3h", "6h", "12h", "24h"] as const; +export const TIMELINE_OPTIONS = ["1h", "3h", "6h", "12h", "24h"] as const;apps/dashboard/app/(app)/logs/components/chart.tsx (1)
150-153: Add block statements for better readabilityFollowing the static analysis suggestion, wrap single-line if statements in blocks for consistency and maintainability.
- if (status >= 200 && status < 300) bucket.success++; - else if (status >= 400 && status < 500) bucket.warning++; - else if (status >= 500) bucket.error++; + if (status >= 200 && status < 300) { + bucket.success++; + } else if (status >= 400 && status < 500) { + bucket.warning++; + } else if (status >= 500) { + bucket.error++; + }🧰 Tools
🪛 Biome (1.9.4)
[error] 150-152: Block statements are preferred in this position.
Unsafe fix: Wrap the statement with a
JsBlockStatement(lint/style/useBlockStatements)
[error] 151-152: Block statements are preferred in this position.
Unsafe fix: Wrap the statement with a
JsBlockStatement(lint/style/useBlockStatements)
[error] 152-152: Block statements are preferred in this position.
Unsafe fix: Wrap the statement with a
JsBlockStatement(lint/style/useBlockStatements)
apps/dashboard/app/(app)/logs/components/log-details/components/log-meta.tsx (1)
3-14: Add prop validation and improve responsive designConsider these improvements:
- Add prop validation to ensure content is always a string
- Make the width responsive instead of fixed
-export const LogMetaSection = ({ content }: { content: string }) => { +import { type FC } from 'react'; + +interface LogMetaSectionProps { + content: string; +} + +export const LogMetaSection: FC<LogMetaSectionProps> = ({ content }) => { return ( <div className="flex justify-between pt-2.5 px-3"> <div className="text-sm text-content/65 font-sans">Meta</div> <Card className="rounded-[5px] flex"> - <CardContent className="text-[12px] w-[300px] flex-2 bg-background-subtle p-3"> + <CardContent className="text-[12px] w-full min-w-[300px] max-w-[500px] flex-2 bg-background-subtle p-3"> <pre>{content}</pre> </CardContent> </Card>apps/dashboard/app/(app)/logs/components/log-details/index.tsx (2)
22-28: Consider using ResizeObserver instead of debounceThe current debounce implementation might cause jank during resizing.
-const PANEL_WIDTH_SET_DELAY = 150; +import { useResizeObserver } from '@/hooks/use-resize-observer'; const _LogDetails = ({ log, onClose, distanceToTop }: Props) => { - const [panelWidth, setPanelWidth] = useState(DEFAULT_DRAGGABLE_WIDTH); - - const debouncedSetPanelWidth = useDebounceCallback((newWidth) => { - setPanelWidth(newWidth); - }, PANEL_WIDTH_SET_DELAY); + const { ref, width } = useResizeObserver<HTMLDivElement>(); + const panelWidth = width ?? DEFAULT_DRAGGABLE_WIDTH;
52-54: Remove unnecessary div and negative marginThe empty div with negative margin seems to be a hack.
<div className="space-y-3 border-b-[1px] border-border py-4"> - <div className="mt-[-24px]" /> <LogSection details={log.request_headers} title="Request Header" />Consider adjusting the padding of the parent container instead if more space is needed.
🛑 Comments failed to post (4)
apps/dashboard/app/(app)/logs/components/chart.tsx (2)
141-141:
⚠️ Potential issueRemove console.log statement
Remove debugging console.log statement before deploying to production.
- console.log(buckets);
94-106:
⚠️ Potential issueFix inconsistency in color variable usage
The Bar components use different color variables (
--color-success) than those defined in chartConfig (--chart-3). This could lead to inconsistent styling.- fill="var(--color-success)" + fill={chartConfig.success.color} - fill="var(--color-warning)" + fill={chartConfig.warning.color} - fill="var(--color-error)" + fill={chartConfig.error.color}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<Bar dataKey="success" stackId="a" fill={chartConfig.success.color} radius={3} /> <Bar dataKey="warning" stackId="a" fill={chartConfig.warning.color} radius={3} /> <Bar dataKey="error" stackId="a" fill={chartConfig.error.color} radius={3} />apps/dashboard/app/(app)/logs/components/log-details/components/log-section.tsx (1)
16-28:
⚠️ Potential issueAdd error handling for malformed detail strings
The current implementation assumes all detail strings contain a colon separator. Add error handling to gracefully handle malformed inputs.
{details.map((header) => { + if (!header.includes(':')) { + console.warn(`Malformed detail string: ${header}`); + return ( + <span key={header}> + <span className="text-content/65">{header}</span> + {"\n"} + </span> + ); + } const [key, ...valueParts] = header.split(":"); const value = valueParts.join(":").trim(); return ( <span key={header}> <span className="text-content/65 capitalize">{key}</span> <span className="text-content whitespace-pre-line"> : {value} </span> {"\n"} </span> ); })}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.{details.map((header) => { if (!header.includes(':')) { console.warn(`Malformed detail string: ${header}`); return ( <span key={header}> <span className="text-content/65">{header}</span> {"\n"} </span> ); } const [key, ...valueParts] = header.split(":"); const value = valueParts.join(":").trim(); return ( <span key={header}> <span className="text-content/65 capitalize">{key}</span> <span className="text-content whitespace-pre-line"> : {value} </span> {"\n"} </span> ); })}apps/dashboard/app/(app)/logs/components/log-details/index.tsx (1)
83-91: 🛠️ Refactor suggestion
Improve type safety in flattenObject utility
The function needs better type safety and null checks.
-function flattenObject(obj: object, prefix = ""): string[] { +function flattenObject(obj: Record<string, unknown>, prefix = ""): string[] { + if (typeof obj !== 'object' || obj === null) { + throw new TypeError('Expected non-null object'); + } return Object.entries(obj).flatMap(([key, value]) => { const newKey = prefix ? `${prefix}.${key}` : key; - if (typeof value === "object" && value !== null) { - return flattenObject(value, newKey); + if (value === null) { + return `${newKey}:null`; + } + if (typeof value === "object") { + return flattenObject(value as Record<string, unknown>, newKey); } return `${newKey}:${value}`; }); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.function flattenObject(obj: Record<string, unknown>, prefix = ""): string[] { if (typeof obj !== 'object' || obj === null) { throw new TypeError('Expected non-null object'); } return Object.entries(obj).flatMap(([key, value]) => { const newKey = prefix ? `${prefix}.${key}` : key; if (value === null) { return `${newKey}:null`; } if (typeof value === "object") { return flattenObject(value as Record<string, unknown>, newKey); } return `${newKey}:${value}`; }); }
|
Moved to here due to too many lock conflicts -> https://github.com/unkeyed/unkey/compare/logs-page-v2 |
What does this PR do?
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
LogsChartcomponent to visualize log data with a bar chart.DatePickerWithRangecomponent for selecting date ranges.ResponseStatuscomponent for filtering logs by HTTP response status codes.SearchComboboxfor managing search items.LogsFilterscomponent.LogDetailscomponents for displaying detailed log information.TimestampInfocomponent for formatting and displaying timestamps.Calendarcomponent for date selection.LogsTablecomponent for displaying logs in a virtualized format.ResizablePanelcomponent for adjustable log detail views.ButtonGroupcomponent for grouping buttons.CommandItemcomponent to support a disabled state.LogMetaSectioncomponent for displaying meta information about logs.LogSectioncomponent for organized display of log details.Bug Fixes
Documentation
Chores