fix: unify datetime components #2876
Conversation
ogzhanolguncu
requested review from
perkinsjr,
chronark,
mcstepp and
MichaelUnkey
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning Rate limit exceeded@ogzhanolguncu has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request removes legacy date-time selection components and consolidates date-time functionality. The 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! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (3)
apps/dashboard/app/(app)/logs/components/controls/components/logs-datetime/index.tsx (1)
1-89: Consider unifying LogsDateTime components.This component is nearly identical to the one in ratelimits. Consider creating a shared component to reduce duplication.
Create a new shared component:
// components/shared/logs-datetime.tsx interface LogsDateTimeProps { namespace?: string; } export const LogsDateTime = ({ namespace }: LogsDateTimeProps) => { // ... shared implementation };apps/dashboard/components/logs/datetime/datetime-popover.tsx (2)
48-56: Add cleanup to prevent memory leaks.The useEffect hook should clean up when the component unmounts.
useEffect(() => { + let mounted = true; const newTitle = since ? OPTIONS.find((s) => s.value === since)?.display ?? initialTitle : startTime ? "Custom" : initialTitle; - onSuggestionChange(newTitle); + if (mounted) { + onSuggestionChange(newTitle); + } + return () => { + mounted = false; + }; }, [since, startTime, initialTitle, onSuggestionChange]);
30-31: Add cleanup for keyboard shortcut.The keyboard shortcut should be cleaned up when the component unmounts.
const [open, setOpen] = useState(false); -useKeyboardShortcut("t", () => setOpen((prev) => !prev)); +useEffect(() => { + const cleanup = useKeyboardShortcut("t", () => setOpen((prev) => !prev)); + return cleanup; +}, []);
🧹 Nitpick comments (9)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/logs-table.tsx (1)
122-123: Consider making the identifier column more responsive.The fixed width percentage (15%) combined with
max-w-40might cause layout issues on different screen sizes. Consider using responsive classes to adjust the max-width based on screen size.- width: "15%", - render: (log) => <div className="font-mono truncate mr-1 max-w-40">{log.identifier}</div>, + width: "auto", + render: (log) => <div className="font-mono truncate mr-1 max-w-32 sm:max-w-40 md:max-w-56">{log.identifier}</div>,internal/ui/src/components/date-time/components/calendar.tsx (2)
90-133: Add more inline documentation for complex date handling logic.While the logic is thorough and handles edge cases well, it would benefit from additional inline documentation explaining the various scenarios, especially in the handleChange function.
Consider adding more detailed comments like this:
const handleChange = (newDate: DateRange | undefined) => { - // No date selected (user cleared the selection) + // Case 1: No date selected (user cleared the selection) + // This happens when the user clicks the clear button or deselects all dates if (!newDate) { onDateChange({ from: undefined, to: undefined }); return; } - // End date was moved later than current end date - // This resets the "from" date while keeping the new "to" date + // Case 2: End date was moved later than current end date + // This resets the "from" date while keeping the new "to" date + // Example: Current range is Jan 1-5, user selects Jan 10 + // Result: Range becomes undefined-Jan 10 if ( date?.from && date?.to && newDate?.to instanceof Date && newDate.to.getTime() > date.to.getTime() ) { onDateChange({ from: undefined, to: newDate.to }); return; }
135-169: Refactor to avoid duplicating the disabled prop.The disabled prop is duplicated in both range and single modes. Consider moving it to commonProps to maintain DRY principles.
const commonProps = { - disabled: minDateRange && { before: minDateRange, after: maxDateRange }, + disabled: (date) => { + // First check the date range constraints + if (minDateRange && (date < minDateRange || date > maxDateRange)) { + return true; + } + // Then check for future dates + return date > new Date(); + }, showOutsideDays, className: cn("", className), classNames: { ...styleClassNames, ...classNames, }, components: { Caption: CustomCaptionComponent, }, ...props, };Then remove the duplicate disabled props:
<DayPicker {...commonProps} mode="range" selected={date} - disabled={(date) => date > new Date()} onSelect={(date: DateRange | undefined) => (date ? handleChange(date) : undefined)} /> <DayPicker {...commonProps} mode="single" selected={singleDay} - disabled={(date) => date > new Date()} onSelect={(date: Date | undefined) => handleSingleChange(date)} />apps/dashboard/components/logs/datetime/constants.ts (1)
3-100: Consider adding type validation for time unit values.The time unit values use inconsistent formats (m, h, d, w). Consider adding validation or using an enum to ensure consistency.
+export type TimeUnitValue = "1m" | "5m" | "15m" | "30m" | "1h" | "3h" | "6h" | "12h" | "24h" | "2d" | "3d" | "1w" | "2w" | "3w" | "4w"; export const OPTIONS: OptionsType = [ { id: 1, - value: "1m", + value: "1m" as TimeUnitValue, display: "Last minute", checked: false, }, // ... apply to all optionsapps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-datetime/index.tsx (2)
12-16: Consider using a default value instead of useEffect.The current implementation might cause a brief flicker showing "Loading...". Consider setting the default value directly in useState.
-const [title, setTitle] = useState<string | null>(null); +const [title, setTitle] = useState<string>("Last 12 hours"); -useEffect(() => { - if (!title) { - setTitle("Last 12 hours"); - } -}, [title]);
18-27: Extract filter field names to constants.The hardcoded filter field names could lead to maintenance issues if they need to change.
+const TIME_FILTER_FIELDS = ["startTime", "endTime", "since"] as const; const timeValues = filters - .filter((f) => ["startTime", "endTime", "since"].includes(f.field)) + .filter((f) => TIME_FILTER_FIELDS.includes(f.field)) .reduce( (acc, f) => ({ ...acc, [f.field]: f.value, }), {}, );apps/dashboard/lib/utils.ts (1)
133-158: LGTM! Consider adding JSDoc documentation.The addition of the week unit and its implementation is correct. The regex pattern and error message have been properly updated to reflect this change.
Consider adding JSDoc documentation to clarify the supported time units and provide examples:
+/** + * Converts a relative time string to a timestamp + * @param relativeTime - A string in the format "number[whdm]" where: + * w = weeks, h = hours, d = days, m = minutes + * @example + * "1w" // 1 week ago + * "1w2d" // 1 week and 2 days ago + * "24h" // 24 hours ago + * @returns timestamp in milliseconds + */ export const getTimestampFromRelative = (relativeTime: string): number => {apps/dashboard/lib/trpc/routers/ratelimit/llm-search/utils.ts (1)
237-246: Consider adding more error handling examples.The error handling examples could be more comprehensive to cover additional edge cases and help the model better understand how to handle ambiguous queries.
Consider adding examples for:
- Multiple ambiguous status terms in one query
- Invalid time formats
- Malformed request IDs
Would you like me to generate additional error handling examples?
apps/dashboard/components/logs/datetime/suggestions.tsx (1)
44-70: Enhance keyboard navigation accessibility.While the keyboard navigation is comprehensive, consider adding the following improvements:
- Support for
HomeandEndkeys to jump to the first/last options.- Support for
PageUpandPageDownfor faster navigation.Here's how you can enhance the keyboard navigation:
const handleKeyDown = (e: KeyboardEvent, index: number) => { switch (e.key) { + case "Home": { + e.preventDefault(); + itemRefs.current[0]?.focus(); + setFocusedIndex(0); + scrollIntoView(0); + break; + } + case "End": { + e.preventDefault(); + const lastIndex = options.length - 1; + itemRefs.current[lastIndex]?.focus(); + setFocusedIndex(lastIndex); + scrollIntoView(lastIndex); + break; + } + case "PageUp": { + e.preventDefault(); + const newIndex = Math.max(0, index - 5); + itemRefs.current[newIndex]?.focus(); + setFocusedIndex(newIndex); + scrollIntoView(newIndex); + break; + } + case "PageDown": { + e.preventDefault(); + const newIndex = Math.min(options.length - 1, index + 5); + itemRefs.current[newIndex]?.focus(); + setFocusedIndex(newIndex); + scrollIntoView(newIndex); + break; + } // ... existing cases } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
apps/dashboard/app/(app)/logs/components/controls/components/logs-datetime/components/datetime-popover.tsx(0 hunks)apps/dashboard/app/(app)/logs/components/controls/components/logs-datetime/components/suggestions.tsx(0 hunks)apps/dashboard/app/(app)/logs/components/controls/components/logs-datetime/index.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/controls/components/logs-datetime/utils/process-time.ts(0 hunks)apps/dashboard/app/(app)/logs/components/table/logs-table.tsx(1 hunks)apps/dashboard/app/(app)/logs/hooks/use-filters.ts(2 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/index.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-datetime/components/suggestions.tsx(0 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-datetime/index.tsx(2 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-search/index.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/logs-table.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/hooks/use-filters.ts(2 hunks)apps/dashboard/components/logs/datetime/constants.ts(1 hunks)apps/dashboard/components/logs/datetime/datetime-popover.tsx(2 hunks)apps/dashboard/components/logs/datetime/suggestions.tsx(1 hunks)apps/dashboard/lib/trpc/routers/ratelimit/llm-search/utils.ts(8 hunks)apps/dashboard/lib/utils.ts(1 hunks)internal/ui/src/components/date-time/components/calendar.tsx(3 hunks)
💤 Files with no reviewable changes (4)
- apps/dashboard/app/(app)/logs/components/controls/components/logs-datetime/utils/process-time.ts
- apps/dashboard/app/(app)/logs/components/controls/components/logs-datetime/components/suggestions.tsx
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-datetime/components/suggestions.tsx
- apps/dashboard/app/(app)/logs/components/controls/components/logs-datetime/components/datetime-popover.tsx
✅ Files skipped from review due to trivial changes (2)
- apps/dashboard/app/(app)/logs/components/table/logs-table.tsx
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-search/index.tsx
🧰 Additional context used
📓 Learnings (1)
apps/dashboard/app/(app)/logs/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-01-30T20:38:00.058Z
Learning: In the logs dashboard, keyboard shortcuts that toggle UI elements (like popovers) should be implemented in the component that owns the state being toggled, not in the presentational wrapper components. For example, the 'T' shortcut for toggling the datetime filter is implemented in DatetimePopover, not in LogsDateTime.
🪛 Biome (1.9.4)
apps/dashboard/components/logs/datetime/suggestions.tsx
[error] 86-87: The elements with the following roles can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/logs-table.tsx (1)
123-123: LGTM! Good UI improvement.Adding
max-w-40class to constrain the identifier's width prevents text overflow and improves table readability.internal/ui/src/components/date-time/components/calendar.tsx (2)
4-5: LGTM! Properly documented linting override.The biome-ignore comment is well-documented and follows the correct format.
73-89: LGTM! Well-structured component with proper type definitions.The Calendar component is well-implemented with:
- Clear prop types
- Proper context usage
- Good state management
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/hooks/use-filters.ts (1)
1-1: LGTM! Good refactoring to centralize validation logic.The changes improve code maintainability by reusing the centralized validation logic from
getTimestampFromRelative.Also applies to: 19-24
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/index.tsx (1)
59-59: LGTM! Good defensive programming.The addition of optional chaining prevents potential runtime errors when accessing
originalTimestamp.apps/dashboard/app/(app)/logs/hooks/use-filters.ts (1)
1-1: LGTM! Consistent with ratelimits implementation.The changes maintain consistency in relative time validation across the application.
Also applies to: 21-24
apps/dashboard/lib/trpc/routers/ratelimit/llm-search/utils.ts (3)
44-44: LGTM! Clear and consistent error message examples.The error message examples have been updated to use the new "blocked/passed" terminology, providing clear guidance to users.
Also applies to: 48-48
84-84: LGTM! Clear status field constraints and system role description.The status field constraints and system role description have been updated to consistently use "blocked/passed" terminology.
Also applies to: 91-91
20-20: Verify the OpenAI model name.The model name "gpt-4o-mini" appears to be incorrect. OpenAI's model names typically don't include the 'o' character.
- model: "gpt-4o-mini", + model: "gpt-4", // or another valid OpenAI model that supports structured outputsapps/dashboard/components/logs/datetime/suggestions.tsx (1)
1-20: LGTM! Well-structured component setup.The component's props interface and state management are well-defined, with proper TypeScript types and appropriate use of refs for DOM manipulation.
|
From Oguzhan Olguncu ‣ @perkinsjr This also includes fix for ratelimit prompt |
|
Oh awesome! I was just about to take a look... |
|
From Oguzhan Olguncu ‣ Give it another try.I haven't tested it thorough. Issues was it was trying use rejected and succeded as a status |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/dashboard/components/logs/datetime/constants.ts (1)
96-96:⚠️ Potential issueAdd null check for CUSTOM_OPTION_ID.
The find operation could return undefined, potentially causing runtime issues.
-export const CUSTOM_OPTION_ID = OPTIONS.find((o) => o.value === undefined)?.id; +export const CUSTOM_OPTION_ID = OPTIONS.find((o) => o.value === undefined)?.id ?? 16;
🧹 Nitpick comments (1)
apps/dashboard/components/logs/datetime/constants.ts (1)
3-94: Consider improvements to the OPTIONS array structure.A few suggestions to enhance maintainability and flexibility:
- Consider making time ranges configurable through environment variables or a configuration file.
- Use meaningful constants for IDs instead of sequential numbers to make reordering safer.
- Add validation for the time range format to ensure consistency.
- Consider setting a default selected option.
Example refactor for using constants for IDs:
+export const TimeRangeIds = { + ONE_MINUTE: 'one_minute', + FIVE_MINUTES: 'five_minutes', + // ... other constants + CUSTOM: 'custom' +} as const; export const OPTIONS: OptionsType = [ { - id: 1, + id: TimeRangeIds.ONE_MINUTE, value: "1m", display: "Last minute", checked: false, }, // ... other options ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/components/logs/datetime/constants.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
apps/dashboard/components/logs/datetime/constants.ts (1)
1-2: LGTM!Good use of TypeScript types for better type safety and code maintainability.
|
From Oguzhan Olguncu ‣ It's now blocked or passed |
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
Style
Refactor