Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/log-details/index.tsx (1)
61-64: 🛠️ Refactor suggestionHandle timezone issues with date parsing more robustly
The current implementation has a specific regex replace for a timezone issue (
3NZto3Z), which is fragile and may break with future API changes or different time formats.- const createdAt = metaData?.createdAt - ? formatDate(metaData.createdAt.replace(/3NZ$/, "3Z")) - : "N/A"; + const createdAt = metaData?.createdAt + ? formatDate(normalizeISOTimestamp(metaData.createdAt)) + : "N/A"; +// Add this helper function +const normalizeISOTimestamp = (timestamp: string): string => { + // Check if the timestamp follows ISO 8601 format + if (!/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}/.test(timestamp)) { + return timestamp; + } + + // Normalize timezone part + return timestamp.replace(/(\d)([A-Z]{2})Z$/, "$1Z"); +};This approach handles more timezone edge cases while keeping the specific fix you needed.
🧹 Nitpick comments (13)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/outcome-popover.tsx (4)
16-23: Consider adding null/undefined check for outcomeCounts.The code filters entries correctly, but doesn't handle cases where
outcomeCountsmight be undefined or null. Consider adding a defensive check.- export const OutcomesPopover = ({ outcomeCounts, isSelected }: OutcomesPopoverProps) => { + export const OutcomesPopover = ({ outcomeCounts = {}, isSelected }: OutcomesPopoverProps) => { const allOutcomeEntries = Object.entries(outcomeCounts).filter(([_, count]) => count > 0);
1-9: Consider organizing imports by source/type.While the imports work correctly, consider organizing them by source (internal vs external) or by type (components vs utilities) for better maintainability.
25-25: Extract CSS constants to a separate file for reusability.The
containerStylestring constant might be useful in other components. Consider moving it to a shared styles file to promote reuse and consistency.
91-92: Consider using template literals with cn utility for className combination.Using string interpolation with backticks alongside
size-[10px]might be better combined using thecn()utility for consistency with the rest of the codebase.- <div - className={`size-[10px] ${getOutcomeColor(outcome)} rounded-[2px] shadow-sm`} - /> + <div + className={cn("size-[10px] rounded-[2px] shadow-sm", getOutcomeColor(outcome))} + />apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/log-details/index.tsx (4)
87-88: Improve metadata display with fallback messageThe current implementation displays "" when metadata is null, but this could be improved with a more descriptive message and better styling.
- const metaString = metaData ? JSON.stringify(metaData, null, 2) : "<EMPTY>"; + const metaString = metaData + ? JSON.stringify(metaData, null, 2) + : "No metadata available for this key";
92-93: Replace shadow-2xl with drop-shadow-2xl for consistencyLine 52 uses
drop-shadow-2xlbut line 92 usesshadow-2xl. For UI consistency, it's better to use the same shadow style.- className="absolute right-0 bg-gray-1 dark:bg-black font-mono shadow-2xl overflow-y-auto z-20 p-4" + className="absolute right-0 bg-gray-1 dark:bg-black font-mono drop-shadow-2xl overflow-y-auto z-20 p-4"
109-121: Add ISO string format option to formatDate functionThe current implementation only formats dates as locale strings, but in some cases, you might want to display ISO format dates for better precision or consistency.
-const formatDate = (date: string | number | Date | null): string => { +const formatDate = (date: string | number | Date | null, format: 'locale' | 'iso' = 'locale'): string => { if (!date) { return "N/A"; } try { - if (date instanceof Date) { - return date.toLocaleString(); - } - return new Date(date).toLocaleString(); + const dateObj = date instanceof Date ? date : new Date(date); + if (isNaN(dateObj.getTime())) { + return "Invalid Date"; + } + return format === 'locale' ? dateObj.toLocaleString() : dateObj.toISOString(); } catch { return "Invalid Date"; } };
69-80: Add tooltips for truncated or technical valuesFor complex or potentially long values like rate limits and remaining requests, consider adding tooltips to provide additional context or full values when truncated.
The rate limit and remaining values could benefit from tooltips that explain what they mean or show the full value when truncated, improving user experience for non-technical users.
apps/dashboard/components/navbar-popover.tsx (3)
159-160: Avoid non-null assertions for better type safety.The non-null assertions on
rowVirtualizer!could potentially cause runtime errors if there's any edge case whereuseVirtualis true butrowVirtualizeris null.- style={{ - height: `${rowVirtualizer!.getTotalSize()}px`, - }} + style={{ + height: `${rowVirtualizer?.getTotalSize() || 0}px`, + }} - {rowVirtualizer!.getVirtualItems().map((virtualRow) => { + {rowVirtualizer?.getVirtualItems().map((virtualRow) => {Also applies to: 162-162
142-154: Consider extracting complex conditional styling logic.The conditional styling and layout logic is becoming complex. Consider extracting this into utility functions or variables for better maintainability.
+ // Extract styling logic for clarity + const getContainerStyles = () => { + return { + height: useVirtual ? maxHeight : shouldScroll ? maxHeight : "auto", + maxHeight: maxHeight, + width: "100%", + }; + }; + + const getContainerClassNames = () => { + return cn( + "w-full", + shouldScroll ? "overflow-auto" : "overflow-visible", + useVirtual ? "" : "flex flex-col gap-1", + ); + }; <div ref={listRef} - className={cn( - "w-full", - shouldScroll ? "overflow-auto" : "overflow-visible", - useVirtual ? "" : "flex flex-col gap-1", - )} - style={{ - height: useVirtual ? maxHeight : shouldScroll ? maxHeight : "auto", - maxHeight: maxHeight, - width: "100%", - }} + className={getContainerClassNames()} + style={getContainerStyles()} >
53-55: Consider relative sizing for height calculations.The fixed maximum height of 200px might not be optimal for all screen sizes. Consider using a relative sizing approach based on the viewport height.
- const exactHeight = items.length * 36; - const maxHeight = Math.min(exactHeight, 200); + // Item height in pixels + const ITEM_HEIGHT = 36; + // Maximum number of visible items or use percentage of viewport height + const MAX_VISIBLE_ITEMS = 5.5; + const exactHeight = items.length * ITEM_HEIGHT; + const maxHeight = Math.min(exactHeight, MAX_VISIBLE_ITEMS * ITEM_HEIGHT);apps/dashboard/app/(app)/apis/[apiId]/_overview/components/controls/components/logs-datetime/index.tsx (2)
9-18: Consider simplifying the initial stateThe component initializes
titletonulland then sets it to "Last 12 hours" usinguseEffect. For cleaner code, consider initializing it directly.- const [title, setTitle] = useState<string | null>(null); + const [title, setTitle] = useState<string>("Last 12 hours"); - useEffect(() => { - if (!title) { - setTitle("Last 12 hours"); - } - }, [title]);
69-71: Consider documenting the filter conditionThe code filters out options that end with "m" (minute-based options). Consider adding a brief comment explaining this design decision for better maintainability.
customOptions={DEFAULT_OPTIONS.filter( + // Filter out minute-based options for a cleaner interface (option) => !option.value || !option.value.endsWith("m"), )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/controls/components/logs-datetime/index.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/log-details/index.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/outcome-popover.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/_overview/logs-client.tsx(1 hunks)apps/dashboard/components/logs/datetime/constants.ts(2 hunks)apps/dashboard/components/logs/datetime/datetime-popover.tsx(7 hunks)apps/dashboard/components/navbar-popover.tsx(7 hunks)apps/dashboard/lib/trpc/routers/utils/granularity.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/dashboard/lib/trpc/routers/utils/granularity.ts
- apps/dashboard/app/(app)/apis/[apiId]/_overview/logs-client.tsx
- apps/dashboard/components/logs/datetime/datetime-popover.tsx
🧰 Additional context used
🧠 Learnings (1)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/controls/components/logs-datetime/index.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-03-12T13:07:07.472Z
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 (17)
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Build / Build
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (15)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/outcome-popover.tsx (3)
11-14: Props type definition is clear and concise.The TypeScript interface appropriately defines the component's expected props with descriptive types.
25-42: Clean implementation for single-outcome display.Good use of conditional rendering with early return pattern when there's exactly one non-valid outcome. The badge component includes appropriate styling and accessibility features with the title attribute.
44-107: Well-structured popover implementation for multiple outcomes.The implementation for displaying multiple outcomes is well organized with:
- Good use of the Popover component
- Proper event propagation handling with
e.stopPropagation()- Clean conditional styling based on the
isSelectedstate- Accessible UI with descriptive titles
- Clear presentation of outcome counts and proper pluralization
The component correctly maps through the outcomes and displays them with appropriate styling.
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/log-details/index.tsx (2)
1-10: LGTM! Well-structured importsThe imports are well-organized, separating client directive, local components, and external packages.
31-58: Excellent component structure with appropriate null checksThe component has good error handling with appropriate null checks and early returns, showing fallback UI when necessary.
apps/dashboard/components/navbar-popover.tsx (6)
5-5: Good addition of virtualization support.Adding the
useVirtualizerimport from@tanstack/react-virtualis a solid choice for implementing virtualization to handle large lists efficiently.
28-33: Well-documented new property.The JSDoc comments for the
virtualizationThresholdproperty clearly explain its purpose and default value, making it easier for other developers to understand and use the component.
47-67: Good implementation of conditional virtualization.The code properly determines when to use virtualization based on the item count and threshold. The height calculations and setup for the virtualizer are well thought out.
114-128: Well-implemented focus management.The effect hooks appropriately handle scrolling to focused items and setting initial focus when the popover opens or closes, which is important for keyboard navigation.
236-256: Good title attribute addition for truncated labels.Adding the
titleattribute to show the full text when labels are truncated is a nice accessibility improvement.
141-193: Verify keyboard navigation accessibility with virtualization.While the implementation looks solid, it's important to ensure that keyboard navigation remains fully accessible with the virtualized list. Consider testing with screen readers and keyboard-only navigation to verify accessibility compliance.
apps/dashboard/components/logs/datetime/constants.ts (1)
3-3: LGTM! Improved naming clarityRenaming
OPTIONStoDEFAULT_OPTIONSmakes the purpose of this constant clearer, indicating these are default options that could potentially be overridden. The corresponding update toCUSTOM_OPTION_IDmaintains consistency.Also applies to: 96-96
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/controls/components/logs-datetime/index.tsx (3)
19-28: LGTM! Effective filter extractionGood job extracting relevant time-related values from filters and constructing the timeValues object using array methods.
33-66: LGTM! Well-structured filter handling logicThe date-time change handler properly manages filter states based on different conditions. The approach of first removing existing time filters and then adding new ones ensures clean state transitions.
73-90: LGTM! Accessible button implementationGood implementation of the button with proper accessibility attributes and visual feedback based on the title's state. The keyboard shortcut hint is a nice touch.
I notice from the retrieved learning that keyboard shortcuts for toggling UI elements should be implemented in components that own the state (DatetimePopover), which appears to be followed here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/dashboard/components/logs/overview-charts/overview-area-chart-error.tsx (1)
67-76: Consider implementing actual time placeholders.The footer currently shows "--:--" for all time labels. Consider enhancing this to show realistic time placeholders that give users a better sense of the expected time range, even in error states.
- <div className="h-8 border-t border-b border-gray-4 px-1 py-2 text-accent-9 font-mono text-xxs w-full flex justify-between"> - {Array(5) - .fill(0) - .map((_, i) => ( - // biome-ignore lint/suspicious/noArrayIndexKey: <explanation> - <div key={i} className="z-10"> - --:-- - </div> - ))} - </div> + <div className="h-8 border-t border-b border-gray-4 px-1 py-2 text-accent-9 font-mono text-xxs w-full flex justify-between"> + {Array(5) + .fill(0) + .map((_, i) => { + // biome-ignore lint/suspicious/noArrayIndexKey: Array of fixed length + return ( + <div key={i} className="z-10"> + {i === 0 ? "00:00" : i === 4 ? "23:59" : "--:--"} + </div> + ); + })} + </div>apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx (1)
286-297: Consider adding fallback time markers.When
datais empty, the footer doesn't display any time markers. Consider adding fallback time markers similar to the error component to maintain consistent UI layout.<div className="h-8 border-t border-b border-gray-4 px-1 py-2 text-accent-9 font-mono text-xxs w-full flex justify-between "> {data.length > 0 ? calculateTimePoints( data[0]?.originalTimestamp ?? Date.now(), data.at(-1)?.originalTimestamp ?? Date.now(), ).map((time, i) => ( // biome-ignore lint/suspicious/noArrayIndexKey: <explanation> <div key={i} className="z-10"> {formatTimestampLabel(time)} </div> )) - : null} + : Array(5) + .fill(0) + .map((_, i) => ( + // biome-ignore lint/suspicious/noArrayIndexKey: Array of fixed length + <div key={i} className="z-10"> + {i === 0 ? "00:00" : i === 4 ? "23:59" : "--:--"} + </div> + ))} </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/charts/index.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/loading.tsx(0 hunks)apps/dashboard/app/(app)/settings/root-keys/new/client.tsx(1 hunks)apps/dashboard/components/logs/overview-charts/overview-area-chart-error.tsx(1 hunks)apps/dashboard/components/logs/overview-charts/overview-area-chart-loader.tsx(1 hunks)apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx(6 hunks)
💤 Files with no reviewable changes (1)
- apps/dashboard/app/(app)/apis/[apiId]/loading.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/dashboard/app/(app)/settings/root-keys/new/client.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/dashboard/components/logs/overview-charts/overview-area-chart-loader.tsx
- apps/dashboard/app/(app)/apis/[apiId]/_overview/components/charts/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Go API Local / Test
- 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 (14)
apps/dashboard/components/logs/overview-charts/overview-area-chart-error.tsx (5)
4-6: Well-structured type definition.The
TimeseriesChartErrorPropstype clearly defines the expected props for this component, making it reusable and type-safe.
8-16: Good implementation of default values.The component properly sets default values for
showRightSideandreverseproperties, ensuring consistent behavior even when these optional properties aren't explicitly provided.
28-35: Well-implemented conditional rendering.The component correctly handles the reverse layout by conditionally rendering the metrics color indicators at the beginning of the range label.
44-56: Flexible right-side section implementation.The conditional rendering of the right side section with metric data is well implemented, allowing for different visualization layouts based on the needs of the parent component.
60-64: Clear error state visualization.The centered error message with clean text styling provides a good user experience when data can't be retrieved.
apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx (9)
28-33: Well-defined chart metric type.The
ChartMetrictype definition provides a flexible structure for defining chart metrics with appropriate properties including an optional formatter for customizing value display.
35-41: Comprehensive chart labels type definition.The
TimeseriesChartLabelstype provides a clear and flexible structure for defining chart labels and metrics, with optional properties for layout control.
43-51: Good props interface update.The props interface has been renamed from
LogsTimeseriesAreaChartPropstoTimeseriesAreaChartPropsand now includes the requiredlabelsprop, improving the component's flexibility and reusability.
114-119: Clean error and loading state handling.The error and loading states now utilize dedicated components, passing through the labels props to maintain consistency in the UI. This improves maintainability and separation of concerns.
122-135: Excellent dynamic metrics calculation.The code has been refactored to dynamically calculate statistics for each metric instead of hardcoding specific metrics. This significantly improves flexibility and maintainability.
138-166: Well-implemented dynamic header section.The header section now dynamically renders based on the metrics provided in the labels prop, with proper support for reverse layout. The formatting of min/max values respects custom formatters defined in the metrics.
169-185: Flexible right-side metrics display.The right side of the header now dynamically renders metrics with their values and styling based on the provided
labels.metricsarray, making the component much more adaptable to different data visualization needs.
199-213: Dynamic gradient definitions for chart areas.The code now dynamically generates gradient definitions for each metric, eliminating hardcoded values and improving maintainability.
260-272: Dynamic rendering of chart areas.The component now dynamically renders Area components for each metric defined in the labels, significantly improving the component's flexibility and reducing code duplication.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/dashboard/components/logs/refresh-button/index.tsx (3)
1-1: Consider the cross-feature dependency implications.The
RatelimitOverviewTooltipcomponent is being imported from the ratelimits feature area, which creates a cross-feature dependency. While component reuse is generally good for consistency, consider whether this creates a tight coupling between features that should be more independent.If this tooltip component contains generic functionality, consider moving it to a shared components folder instead.
54-54: Consider using React Fragment instead of div.The div wrapper doesn't seem to have styling or functional purpose. Consider using a React Fragment (
<>...</>) to avoid unnecessary DOM nesting.- <div> + <> <Button ... </Button> - </div> + </>
62-62: Consider extracting styles to design system.The button styling is applied directly via className. For better maintainability and consistency, consider extracting these styles to your design system or using existing style tokens/variants.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/dashboard/app/(app)/settings/root-keys/new/client.tsx(2 hunks)apps/dashboard/components/logs/refresh-button/index.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/app/(app)/settings/root-keys/new/client.tsx
🔇 Additional comments (2)
apps/dashboard/components/logs/refresh-button/index.tsx (2)
49-53: Good addition of contextual feedback.The tooltip provides helpful information to users about why refresh might be unavailable, improving the overall user experience. The disabled logic ensures the tooltip only appears when refresh is unavailable.
59-59: Excellent improvement to keyboard shortcut discoverability.Adding the keyboard shortcut information to the title improves discoverability and provides better UX for users who hover over the button.
2 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
New Features
Improvements