chore: ratelimit overview tooltip move to unkey ui and renamed #3207
chore: ratelimit overview tooltip move to unkey ui and renamed #3207ogzhanolguncu merged 26 commits intomainfrom
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
📝 Walkthrough""" WalkthroughThis change introduces a unified Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UIComponent
participant InfoTooltip
User->>UIComponent: Hover or focus on trigger element
UIComponent->>InfoTooltip: Render with props (content, position, variant, etc.)
InfoTooltip-->>User: Displays tooltip with specified content and styling
User->>InfoTooltip: (Optional) Interact (e.g., click, copy)
InfoTooltip-->>UIComponent: Handles tooltip visibility and disables if needed
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (15)
✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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! 🙏 |
MichaelUnkey
requested review from
chronark,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
@MichaelUnkey we have similar component in the dashboard scattered around. Can you quickly check them out? you the same className to search for similar items |
|
FYI my session refresh stuff (with the change to getOrgId => getAuth is about to be merged, so you'll have to pull from main and rebase soon |
|
Rebase away everyone it is in main. |
…-tooltip-moved-to-ui
…-tooltip-moved-to-ui
![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
🧹 Nitpick comments (5)
internal/ui/src/components/overview-tooltip.tsx (5)
1-4: Linting suppression - consider a longer-term solution.The biome-ignore comment suggests there's a tension with the linter regarding React imports. While this works as a temporary solution, consider addressing this more systematically across the codebase.
6-11: Consider using a theme system for variant styling.The variant styles are defined inline within the component file. For better maintainability and consistency across components, consider moving these to a dedicated theme system or styling constants file.
21-40: Add JSDoc comments to improve component documentation.The component is well-typed but lacks descriptive JSDoc comments. Adding documentation comments would improve developer experience when using this component.
+/** + * A reusable tooltip component that simplifies the usage of tooltips across the application. + * @param variant - Visual style of the tooltip ("primary" or "secondary") + * @param delayDuration - Time in ms before the tooltip appears + * @param content - Content to display inside the tooltip + * @param position - Controls tooltip positioning (side, alignment, offset) + * @param disabled - When true, prevents the tooltip from showing + * @param asChild - When true, merges the trigger with its child element + * @param className - Additional classes to apply to tooltip content + * @param triggerClassName - Additional classes to apply to trigger element + */ const OverviewTooltip = ({
43-43: Improve clarity of the conditional open state implementation.The
open={disabled ? false : undefined}pattern works but isn't immediately intuitive. Consider adding a brief comment explaining this approach for future developers.- <Tooltip open={disabled ? false : undefined}> + {/* When undefined, lets Tooltip manage its own open state. When false, forces it closed */} + <Tooltip open={disabled ? false : undefined}>
48-52: The positioning defaults should be documented in the JSDoc.The component sets default positions that aren't immediately obvious from the props definition. These defaults should be documented in the component's JSDoc to make them clear to users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/override-indicator.tsx(2 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/bar-chart/components/outcome-explainer.tsx(3 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/status-cell/index.tsx(3 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/keys-list.tsx(3 hunks)apps/dashboard/app/(app)/authorization/roles/[roleId]/tree.tsx(3 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/components/override-indicator.tsx(4 hunks)apps/dashboard/components/dashboard/root-key-table/index.tsx(2 hunks)apps/dashboard/components/logs/details/request-response-details.tsx(3 hunks)apps/dashboard/components/logs/llm-search/components/search-example-tooltip.tsx(2 hunks)apps/dashboard/components/logs/queries/list-group.tsx(2 hunks)apps/dashboard/components/logs/queries/queries-overflow-tooltip.tsx(3 hunks)apps/dashboard/components/navigation/sidebar/team-switcher.tsx(2 hunks)apps/dashboard/components/stats-card/index.tsx(2 hunks)internal/ui/src/components/overview-tooltip.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/components/override-indicator.tsx
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/dashboard/components/logs/queries/list-group.tsx (2)
internal/ui/src/components/tooltip.tsx (2)
Tooltip(30-30)TooltipTrigger(30-30)apps/dashboard/lib/utils.ts (1)
cn(5-7)
apps/dashboard/components/logs/queries/queries-overflow-tooltip.tsx (2)
internal/ui/src/components/tooltip.tsx (2)
Tooltip(30-30)TooltipTrigger(30-30)apps/dashboard/components/logs/queries/queries-pill.tsx (1)
QueriesPill(7-19)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Test Go API Local / Test (Shard 5/8)
- GitHub Check: Test Go API Local / Test (Shard 2/8)
- GitHub Check: Test Go API Local / Test (Shard 8/8)
- GitHub Check: Test Go API Local / Test (Shard 3/8)
- GitHub Check: Test Go API Local / Test (Shard 7/8)
- GitHub Check: Test Go API Local / Test (Shard 4/8)
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Test Go API Local / Test (Shard 6/8)
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: autofix
🔇 Additional comments (28)
apps/dashboard/components/dashboard/root-key-table/index.tsx (2)
24-24: Import changes look good!The import has been correctly updated to include the new
OverviewTooltipcomponent from the@unkey/uipackage.
91-103: Clean implementation of the new OverviewTooltip component!The nested tooltip structure has been successfully replaced with a single
OverviewTooltipcomponent while maintaining the same functionality and content. The tooltip still explains that only the first part of the key is shown for security reasons.apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/override-indicator.tsx (2)
5-5: Import statement correctly updated!The import has been properly updated to include only the
OverviewTooltipcomponent from@unkey/ui.
17-19: Good refactoring of the KeyTooltip component!The implementation now uses the new
OverviewTooltipcomponent with appropriate configuration for positioning (side: "right") and theasChildprop to properly wrap children. This simplifies the code while maintaining the same functionality.apps/dashboard/components/navigation/sidebar/team-switcher.tsx (2)
20-20: Import statement correctly updated!The import has been properly updated to include the
OverviewTooltipcomponent from@unkey/ui.
111-120: Well-implemented tooltip for workspace name!The nested tooltip structure has been successfully replaced with a single
OverviewTooltipcomponent. The implementation includes appropriate configuration for variant, positioning, and content styling. The tooltip appears when the workspace name is truncated due to space constraints, which improves the user experience.apps/dashboard/components/logs/details/request-response-details.tsx (3)
3-3: Import statement correctly updated!The import has been properly updated to include the
OverviewTooltipcomponent from@unkey/ui.
74-74: Updated styling for tooltip label!The span element now includes additional styling classes
lg:no-wrap text-leftwhich will improve text layout and readability, especially on larger screens. This ensures the label text is properly aligned with the new tooltip implementation.
86-94: Good implementation of the tooltip in renderField function!The nested tooltip structure has been successfully replaced with a single
OverviewTooltipcomponent. The implementation includes appropriate configuration for positioning, variant, and custom trigger styling. The clickable behavior for copying content to clipboard is preserved.apps/dashboard/components/logs/queries/queries-overflow-tooltip.tsx (2)
1-1: Clean import upgrade to use the new OverviewTooltip component.The import statement has been updated to use the new consolidated tooltip component from the UI library.
14-29: Good refactoring to use the unified OverviewTooltip component.The implementation has been simplified by replacing what was likely a multi-component tooltip structure with a single
OverviewTooltipcomponent. This maintains the same functionality while reducing the component nesting and complexity.apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/keys-list.tsx (2)
6-6: Import statement correctly updated to include OverviewTooltip.The import has been properly updated to include the new consolidated tooltip component.
77-104: Well-implemented conversion to OverviewTooltip for identity information.The identity icon tooltip has been successfully refactored to use the new
OverviewTooltipcomponent with appropriate positioning and styling. The use ofasChildwithReact.cloneElementmaintains the existing behavior while adding cursor styling, ensuring a good user experience.apps/dashboard/components/logs/queries/list-group.tsx (2)
4-4: Import statement correctly updated to use the new OverviewTooltip component.The import has been properly updated to include the consolidated tooltip component.
145-166: Good refactoring of the bookmark button tooltip.The bookmark button tooltip has been successfully converted to use the new
OverviewTooltipcomponent. The implementation maintains all the existing functionality including proper tooltip messaging based on bookmark state, while simplifying the component structure.apps/dashboard/app/(app)/authorization/roles/[roleId]/tree.tsx (2)
6-6: Import statement correctly updated to include OverviewTooltip.The import has been properly updated to include the new consolidated tooltip component.
91-117: Well-implemented tooltip refactoring for permission nodes.The leaf permission nodes tooltip has been successfully converted to use the new
OverviewTooltipcomponent. The implementation includes appropriate positioning, delay duration, and maintains the same functionality for displaying permission details with copy capability.apps/dashboard/components/logs/llm-search/components/search-example-tooltip.tsx (2)
2-2: Correct import for the new unified tooltip component.The import statement correctly references the new
OverviewTooltipcomponent from the@unkey/uipackage, aligning with the PR objective of refactoring tooltips.
20-50: Clean implementation of the refactored tooltip component.The previous nested tooltip components have been successfully replaced with the unified
OverviewTooltipcomponent. The refactoring maintains all functionality while simplifying the code structure:
- Content structure is preserved with proper hierarchy
- Test IDs are maintained (
data-testid="info-icon"and example query test IDs)- Styling classes remain consistent
- Delay duration is preserved (150ms)
- The secondary variant is correctly specified
apps/dashboard/components/stats-card/index.tsx (3)
3-3: Correctly updated import statement.The import has been correctly updated to use the new
OverviewTooltipcomponent from@unkey/ui, removing the individual tooltip-related imports.
35-39: Well-structured tooltip implementation for the name field.The previous tooltip implementation has been properly replaced with the new
OverviewTooltipcomponent, maintaining the same functionality while simplifying the code. The positioning is explicitly set to appear on top, which is good for consistency.
42-46: Consistent tooltip implementation for the secondaryId field.The tooltip for the secondaryId field has been properly implemented using the same pattern as the name field, ensuring consistency throughout the component. The position is correctly set to "top" and the content is properly passed to the tooltip.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/bar-chart/components/outcome-explainer.tsx (2)
3-3: Correctly updated import statement.The import has been correctly updated to use the new
OverviewTooltipcomponent from@unkey/ui, replacing the previous tooltip-related imports.
103-151: Effectively refactored OutcomeExplainer tooltip.The implementation successfully replaces the previous multi-component tooltip structure with the new unified
OverviewTooltipcomponent. The refactoring:
- Maintains the same content structure and styling
- Preserves the delay duration (300ms)
- Uses the
asChildprop correctly to wrap the children- Explicitly sets positioning to bottom
- Uses the secondary variant for consistent styling
- Preserves all the conditional rendering logic for API key activity
This change improves code maintainability without altering the component's behavior.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/status-cell/index.tsx (2)
5-5: Correctly updated import statement.The import has been correctly updated to use the new
OverviewTooltipcomponent from@unkey/ui, replacing the previous tooltip-related imports.
59-139: Successfully simplified StatusDisplay tooltip implementation.The refactoring successfully replaces the previous complex tooltip structure with the new
OverviewTooltipcomponent. The implementation:
- Uses the
asChildprop correctly to wrap the StatusBadge component- Preserves all the conditional rendering logic for different statuses
- Maintains the same styling and content structure
- Removes the need for explicit state management for tooltip visibility
- Preserves the functionality of the enable key mutation button
This change results in cleaner, more maintainable code while keeping all the original functionality intact.
internal/ui/src/components/overview-tooltip.tsx (2)
1-61: Overall, this is a well-designed component that should help standardize tooltip usage.The component effectively consolidates and simplifies tooltip implementations across the codebase. The props API is flexible and well-typed, and the default values are sensible. This should help maintain consistency and reduce duplication.
21-58:Details
✅ Verification successful
Consider adding explicit accessibility attributes.
While the underlying tooltip components may handle accessibility, it's worth considering whether additional ARIA attributes should be passed through as props for more complex use cases.
🏁 Script executed:
#!/bin/bash # Check if the imported tooltip components already handle accessibility attributes grep -r "aria-" --include="*.tsx" internal/ui/src/components/tooltipLength of output: 136
🏁 Script executed:
#!/bin/bash # Locate tooltip component definitions echo "=== Tooltip component files ===" rg -l "TooltipProvider" -n internal/ui/src/components rg -l "TooltipTrigger" -n internal/ui/src/components rg -l "TooltipContent" -n internal/ui/src/components echo echo "=== Searching for explicit aria- attributes in UI components ===" rg -n "aria-" internal/ui/src/componentsLength of output: 3550
🏁 Script executed:
#!/bin/bash # Display the beginning of tooltip.tsx to inspect implementation echo "=== tooltip.tsx (head) ===" sed -n '1,200p' internal/ui/src/components/tooltip.tsx || echo "Failed to read tooltip.tsx" echo echo "=== aria- usage in tooltip.tsx ===" rg -n "aria-" internal/ui/src/components/tooltip.tsx || echo "No aria- attributes found in tooltip.tsx"Length of output: 1528
Accessibility is handled by Radix UI’s Tooltip primitives—no changes needed
Radix’s
<TooltipPrimitive.Trigger>and<TooltipPrimitive.Content>automatically apply the necessaryrole="tooltip",aria-describedby, and other ARIA attributes out of the box. Since ourOverviewTooltipwrapper simply delegates to those primitives without stripping their defaults, no additional attributes are required for standard use cases.If you do have more advanced requirements (e.g. custom
aria-labels oraria-describedbyoverrides), consider extending the wrapper’s API to accept and spread extra props:• Add optional
triggerProps?: ComponentProps<typeof TooltipTrigger>andcontentProps?: ComponentProps<typeof TooltipContent>toOverviewTooltip’s props.
• Spread them on the corresponding primitives:<TooltipTrigger {...triggerProps} asChild={asChild} className={triggerClassName}> {children} </TooltipTrigger> … <TooltipContent {...contentProps} className={cn(variants[variant], className)} …> {content} </TooltipContent>But for typical scenarios, the current implementation is already fully accessible.
|
I think we should work on that name. The reason why it's called RatelimitOverview was because that's being used for Overview of Ratelimit requests. I think those two are better options: InfoTooltip wdyt? |
Pull request was converted to draft
.../(app)/ratelimits/[namespaceId]/_overview/components/table/components/override-indicator.tsx
Show resolved
Hide resolved
...dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/keys-list.tsx
Show resolved
Hide resolved
.../apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/status-cell/index.tsx
Show resolved
Hide resolved
...ashboard/app/(app)/apis/[apiId]/_overview/components/table/components/override-indicator.tsx
Show resolved
Hide resolved
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/components/table/logs-table.tsx
Show resolved
Hide resolved
...Id]/keys/[keyAuthId]/_components/components/table/components/actions/components/key-info.tsx
Show resolved
Hide resolved
...eyAuthId]/_components/components/table/components/bar-chart/components/outcome-explainer.tsx
Show resolved
Hide resolved
|
Dashboard looks okay. The only thing that bothers me is this file |
4 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
Refactor
Style
Documentation
Accessibility