Conversation
MichaelUnkey
requested review from
chronark,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
📝 WalkthroughWalkthroughThis update replaces all local imports of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AppComponent
participant TimestampInfo (from @unkey/ui)
participant Tooltip
User->>AppComponent: Interacts with timestamp display
AppComponent->>TimestampInfo: Renders with timestamp value and displayType
TimestampInfo->>Tooltip: Displays formatted timestamp
User->>Tooltip: Clicks on value to copy
Tooltip->>User: Shows "Copied!" feedback
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🪛 LanguageToolapps/engineering/content/design/components/button.mdx[style] ~1057-~1057: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase. (EN_WEAK_ADJECTIVE) ⏰ Context from checks skipped due to timeout of 90000ms (18)
🔇 Additional comments (3)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
|
@MichaelUnkey can you update our engineering docs too? |
|
if you haven't already 😄 |
|
I meant changes for Button 🙏 |
|
Yup on the list. Seperate PR just wanted to finish the one I started first |
![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
🧹 Nitpick comments (9)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/logs-table.tsx (1)
10-10: Consolidate@unkey/uiimports
You have two separate imports from@unkey/ui. Combining them improves readability and reduces duplication:-import { TimestampInfo } from "@unkey/ui"; -import { Button, Empty } from "@unkey/ui"; +import { TimestampInfo, Button, Empty } from "@unkey/ui";internal/ui/src/components/button.tsx (1)
153-183: Fix comment label for info variant.The comment on line 153 says "// Success" but it should say "// Info" since it's defining the info variant compounds.
- // Success + // Info { variant: "primary", color: "info",The styling implementation for the info variant looks good across all button types (primary, outline, ghost).
apps/engineering/content/design/components/timestamp-info.mdx (1)
60-60: Add missing article "the" before "clipboard".Minor grammatical fix needed.
-Each value can be copied to clipboard by clicking on it. +Each value can be copied to the clipboard by clicking on it.🧰 Tools
🪛 LanguageTool
[uncategorized] ~60-~60: Possible missing article found.
Context: ...tamp value Each value can be copied to clipboard by clicking on it. ## Accessibility -...(AI_HYDRA_LEO_MISSING_THE)
apps/engineering/content/design/components/timestamp-example.tsx (2)
12-37: Consider consistent wrapper structure across examplesI notice that only this first example has an extra
<div className="flex flex-col gap-4">wrapper that's not present in the other two examples.- return ( - <div className="flex flex-col gap-4"> - <RenderComponentWithSnippet> + return ( + <RenderComponentWithSnippet>
5-107: Consider refactoring to reduce code duplicationAll three components have nearly identical structure and only differ in the
displayTypeprop. Consider creating a single component that accepts the display type as a prop.export const TimestampExample = ({ displayType }: { displayType: "local" | "utc" | "relative" }) => { const now = Date.now(); const oneHourAgo = now - 3600 * 1000; const oneDayAgo = now - 86400 * 1000; const oneWeekAgo = now - 604800 * 1000; return ( <RenderComponentWithSnippet> <Row> {/* Current Time */} <div className="flex flex-col gap-2 text-center"> <h3 className="text-sm font-medium">Current Time</h3> <TimestampInfo value={now} displayType={displayType} /> </div> {/* One Hour Ago */} <div className="flex flex-col gap-2 text-center"> <h3 className="text-sm font-medium">One Hour Ago</h3> <TimestampInfo value={oneHourAgo} displayType={displayType} /> </div> {/* One Day Ago */} <div className="flex flex-col gap-2 text-center"> <h3 className="text-sm font-medium">One Day Ago</h3> <TimestampInfo value={oneDayAgo} displayType={displayType} /> </div> {/* One Week Ago */} <div className="flex flex-col gap-2 text-center"> <h3 className="text-sm font-medium">One Week Ago</h3> <TimestampInfo value={oneWeekAgo} displayType={displayType} /> </div> </Row> </RenderComponentWithSnippet> ); }; export const TimestampExampleLocalTime = () => <TimestampExample displayType="local" />; export const TimestampExampleUTC = () => <TimestampExample displayType="utc" />; export const TimestampExampleRelative = () => <TimestampExample displayType="relative" />;internal/ui/src/components/timestamp-info.tsx (4)
41-62: Remove duplicated type definition in propsThe props type is defined twice - once in the component type definition and again in the parameter destructuring. This is redundant and could lead to inconsistencies if one is updated but not the other.
export const TimestampInfo: React.FC<{ value: string | number; className?: string; displayType?: DisplayType; triggerRef?: React.RefObject<HTMLElement>; open?: boolean; onOpenChange?: (open: boolean) => void; }> = ({ value, className, displayType = "local", triggerRef: externalTriggerRef, open, onOpenChange, -}: { - className?: string; - value: string | number; - displayType?: DisplayType; - triggerRef?: React.RefObject<HTMLElement>; - open?: boolean; - onOpenChange?: (open: boolean) => void; -}) => { +}) => {
106-125: Add keyboard accessibility to the TooltipRow componentThe TooltipRow component is currently only clickable with a mouse. For better accessibility, consider adding keyboard support.
//biome-ignore lint/a11y/useKeyWithClickEvents: no need <span onClick={(e) => { e.stopPropagation(); navigator.clipboard.writeText(value); setCopied(true); setTimeout(() => setCopied(false), 1000); }} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + e.stopPropagation(); + navigator.clipboard.writeText(value); + setCopied(true); + setTimeout(() => setCopied(false), 1000); + } + }} + tabIndex={0} + role="button" className="flex items-center hover:bg-gray-3 text-left cursor-pointer w-full px-5 py-2" >
13-17: Consider a more flexible way to detect microsecond timestampsThe function
isUnixMicrochecks for exactly 16 digits, which might not be flexible enough for all use cases. Consider a more robust approach.const isUnixMicro = (unix: string | number): boolean => { - const digitLength = String(unix).length === 16; + // Check if length is in microsecond range (typically 16 digits for current dates) + const strUnix = String(unix); + const digitLength = strUnix.length >= 15 && strUnix.length <= 17; const isNum = !Number.isNaN(Number(unix)); return isNum && digitLength; };
106-125: Consider moving TooltipRow out of the parent componentThe
TooltipRowcomponent is defined inside theTimestampInfocomponent, causing it to be recreated on each render. Consider moving it outside to optimize performance.Extract the
TooltipRowcomponent outside of theTimestampInfocomponent to avoid unnecessary recreation on each render. This would be particularly beneficial if the component grows more complex in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
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/logs-table.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/last-used.tsx(1 hunks)apps/dashboard/app/(app)/audit/components/table/log-details/components/log-footer.tsx(1 hunks)apps/dashboard/app/(app)/audit/components/table/logs-table.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/table/log-details/components/log-footer.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/table/logs-table.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/logs-table.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/log-details/components/log-footer.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/logs-table.tsx(1 hunks)apps/dashboard/components/logs/control-cloud/control-pill.tsx(1 hunks)apps/engineering/content/design/components/timestamp-example.tsx(1 hunks)apps/engineering/content/design/components/timestamp-info.mdx(1 hunks)internal/ui/src/components/button.tsx(2 hunks)internal/ui/src/components/timestamp-info.tsx(1 hunks)internal/ui/src/index.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/engineering/content/design/components/timestamp-example.tsx (2)
apps/engineering/app/components/render.tsx (1)
RenderComponentWithSnippet(12-67)internal/ui/src/components/timestamp-info.tsx (1)
TimestampInfo(41-156)
internal/ui/src/components/timestamp-info.tsx (1)
internal/ui/src/components/tooltip.tsx (3)
Tooltip(30-30)TooltipTrigger(30-30)TooltipContent(30-30)
🪛 LanguageTool
apps/engineering/content/design/components/timestamp-info.mdx
[uncategorized] ~48-~48: Loose punctuation mark.
Context: ...e changes | ## Display Types - local: Shows time in the user's local timezone...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~60-~60: Possible missing article found.
Context: ...tamp value Each value can be copied to clipboard by clicking on it. ## Accessibility -...
(AI_HYDRA_LEO_MISSING_THE)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Test Go API Local / Test (Shard 5/8)
- GitHub Check: Test Go API Local / Test (Shard 4/8)
- GitHub Check: Test Go API Local / Test (Shard 3/8)
- GitHub Check: Test Go API Local / Test (Shard 2/8)
- GitHub Check: Build / Build
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test ./internal/clickhouse
🔇 Additional comments (21)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/logs-table.tsx (1)
8-8: Use centralized TimestampInfo component
ImportingTimestampInfofrom@unkey/uiensures that the timestamp rendering is consistent across all dashboard tables instead of relying on a local copy.apps/dashboard/app/(app)/audit/components/table/logs-table.tsx (1)
6-6: Switch to shared TimestampInfo import
TheTimestampInfocomponent is now sourced from the central UI library (@unkey/ui), maintaining uniform behavior and look across audit logs.apps/dashboard/app/(app)/logs/components/table/logs-table.tsx (1)
9-9: Centralize TimestampInfo import
Updating the import to use the sharedTimestampInfofrom@unkey/uiimproves reuse and ensures consistent timestamp behavior in the logs table.apps/dashboard/components/logs/control-cloud/control-pill.tsx (1)
3-3: Import TimestampInfo from UI library
Switching to@unkey/uiforTimestampInfoaligns with the refactor to use the centralized component for time formatting in filter pills.apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/logs-table.tsx (1)
11-11: Adopt shared TimestampInfo component
The replacement of the local timestamp helper with the@unkey/uiTimestampInfocomponent standardizes the last-request column across rate-limit tables.apps/dashboard/app/(app)/logs/components/table/log-details/components/log-footer.tsx (1)
8-8: Centralized import ofTimestampInfo
Switching the import to@unkey/uistandardizes the timestamp component usage across the app.No additional logic changes are required; ensure the package is installed as noted in other reviews.
apps/dashboard/app/(app)/audit/components/table/log-details/components/log-footer.tsx (1)
6-6: Centralized import ofTimestampInfo
This import now points to the shared UI package, reducing duplication of the local component.Ensure
@unkey/uiis updated in your dependencies if not already present.apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/log-details/index.tsx (1)
5-5: Centralized import ofTimestampInfo
Replacing the local import with the shared UI package aligns this file with the rest of the codebase.Double-check that the version of
@unkey/uiyou’ve added exports this component as expected.apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/last-used.tsx (1)
5-5: LGTM! Replaced local import with centralized UI component.The import change from a local path to the shared
@unkey/uipackage properly centralizes the UI component and follows best practices for component reusability.internal/ui/src/index.ts (1)
9-9: LGTM! Component export added to UI library.The
TimestampInfocomponent is now properly exported from the UI library, making it available for use across the application.internal/ui/src/components/button.tsx (1)
42-42: LGTM! Added info color variant to button options.The "info" color variant is correctly added to the button's color options.
apps/engineering/content/design/components/timestamp-info.mdx (1)
1-67: LGTM! Well-documented component with comprehensive usage examples.The documentation for the TimestampInfo component is thorough and covers all key aspects including usage examples, props, display types, and accessibility.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~48-~48: Loose punctuation mark.
Context: ...e changes | ## Display Types -local: Shows time in the user's local timezone...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~60-~60: Possible missing article found.
Context: ...tamp value Each value can be copied to clipboard by clicking on it. ## Accessibility -...(AI_HYDRA_LEO_MISSING_THE)
apps/engineering/content/design/components/timestamp-example.tsx (4)
1-4: Import organization looks goodThe component correctly imports
TimestampInfofrom the@unkey/uipackage, which aligns with the PR objective of centralizing UI components.
5-39: TimestampExampleLocalTime component implementation is cleanThe component demonstrates the local time format nicely with a clear hierarchy of timestamps, from current time to one week ago. The implementation correctly uses the
displayType="local"prop.
41-73: TimestampExampleUTC component looks goodThe UTC example follows the same pattern as the local time example and correctly uses
displayType="utc".
75-107: TimestampExampleRelative component looks goodThe relative time example matches the pattern of the previous examples and correctly uses
displayType="relative".internal/ui/src/components/timestamp-info.tsx (5)
1-8: Import organization is goodThe component imports necessary dependencies and uses correct React imports.
9-17: Unix timestamp handling functions look goodThe utility functions for converting and detecting microsecond Unix timestamps are well implemented.
19-37: Timestamp formatters implemented correctlyAll three timestamp formatters (local, UTC, and relative) correctly handle both regular and microsecond Unix timestamps.
76-91: Good handling of tooltip positioningThe dynamic positioning of the tooltip based on viewport position is well implemented with proper event listeners and cleanup.
127-155: Clean, well-structured component renderingThe rendering logic nicely handles both external and internal trigger references, and the tooltip content is well organized.
...p/(app)/ratelimits/[namespaceId]/logs/components/table/log-details/components/log-footer.tsx
Show resolved
Hide resolved
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