chore: dialog and dialog container to ui#3213
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis change migrates all usages of the Changes
Sequence Diagram(s)sequenceDiagram
participant ConsumerComponent as Any Dashboard Component
participant @unkey/ui as @unkey/ui/DialogContainer
participant RadixDialog as Radix UI Dialog Primitives
ConsumerComponent->>@unkey/ui: Import DialogContainer
ConsumerComponent->>@unkey/ui: Render <DialogContainer ...props />
@unkey/ui->>RadixDialog: Use Radix Dialog primitives for modal logic
@unkey/ui->>ConsumerComponent: Render dialog with header, content, footer, and handle open/close events
Possibly related PRs
Suggested reviewers
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
![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 (6)
apps/dashboard/app/(app)/apis/[apiId]/settings/components/delete-protection.tsx (1)
6-8: Approve import update; consider consolidating@unkey/uiimports
TheDialogContainerimport has been correctly switched to the shared UI library. To reduce duplication and improve readability, you can merge the three separate@unkey/uiimports into a single statement:-from "@unkey/ui"; +import { DialogContainer, InlineLink, Input, SettingCard, Button } from "@unkey/ui";apps/dashboard/app/auth/sign-in/org-selector.tsx (1)
3-6: Approve import update; consolidate@unkey/uiimports
TheDialogContainerimport from@unkey/uiis correct. You may also merge the two separate@unkey/uiimports into one for clarity:-import { DialogContainer } from "@unkey/ui"; -import { Button } from "@unkey/ui"; +import { DialogContainer, Button } from "@unkey/ui";apps/dashboard/app/(app)/ratelimits/[namespaceId]/_components/delete-dialog.tsx (1)
6-7: Approve import update; consolidate@unkey/uiimports
The update to importDialogContainerfrom@unkey/uiis correct. For consistency, combine both imports into a single one:-import { DialogContainer } from "@unkey/ui"; -import { Button, Input } from "@unkey/ui"; +import { DialogContainer, Button, Input } from "@unkey/ui";apps/engineering/content/design/components/dialog-container.mdx (1)
34-34: Nit: unify spelling of “autofocus”
In the props table, consider using “autofocus” (one word) instead of “auto-focus” to align with common conventions and the prop namepreventAutoFocus.🧰 Tools
🪛 LanguageTool
[misspelling] ~34-~34: This word is normally spelled as one.
Context: ... | false | Whether to prevent auto-focus on open | | children ...(EN_COMPOUNDS_AUTO_FOCUS)
apps/engineering/content/design/components/dialog-container.example.tsx (1)
25-45: Consider refining theonOpenChangehandler implementation.The current implementation toggles the state rather than using the provided value.
- onOpenChange={() => setIsOpen(!isOpen)} + onOpenChange={(open) => setIsOpen(open)}This approach is more in line with controlled component patterns and prevents potential state mismatches.
internal/ui/src/components/dialog/dialog-container.tsx (1)
20-30: Consider documenting thepreventAutoFocusbehavior.The
preventAutoFocusprop defaults to true, which may impact accessibility for keyboard users. Consider adding a comment explaining this choice.- preventAutoFocus = true, + // Default to preventing autofocus to avoid unexpected focus shifts + // Set to false when keyboard accessibility is a priority + preventAutoFocus = true,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
apps/dashboard/app/(app)/apis/[apiId]/settings/components/delete-api.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/settings/components/delete-protection.tsx(1 hunks)apps/dashboard/app/(app)/authorization/_components/rbac-form.tsx(1 hunks)apps/dashboard/app/(app)/authorization/permissions/[permissionId]/delete-permission.tsx(1 hunks)apps/dashboard/app/(app)/authorization/roles/[roleId]/delete-role.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/_components/delete-dialog.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/_components/identifier-dialog.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/_components/namespace-delete-dialog.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/_components/create-namespace-button.tsx(1 hunks)apps/dashboard/app/(app)/settings/billing/components/confirmation.tsx(1 hunks)apps/dashboard/app/(app)/settings/team/invite.tsx(1 hunks)apps/dashboard/app/auth/sign-in/org-selector.tsx(1 hunks)apps/engineering/content/design/components/dialog-container.example.tsx(1 hunks)apps/engineering/content/design/components/dialog-container.mdx(1 hunks)internal/ui/package.json(1 hunks)internal/ui/src/components/dialog/dialog-container.tsx(1 hunks)internal/ui/src/components/dialog/dialog-parts.tsx(1 hunks)internal/ui/src/components/dialog/dialog.tsx(1 hunks)internal/ui/src/index.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
internal/ui/src/components/dialog/dialog-container.tsx (3)
internal/ui/src/components/dialog/dialog.tsx (2)
Dialog(152-152)DialogContent(157-157)apps/dashboard/lib/utils.ts (1)
cn(5-7)internal/ui/src/components/dialog/dialog-parts.tsx (3)
DefaultDialogHeader(19-30)DefaultDialogContentArea(36-45)DefaultDialogFooter(51-59)
internal/ui/src/components/dialog/dialog-parts.tsx (2)
internal/ui/src/components/dialog/dialog-container.tsx (3)
DefaultDialogHeader(52-52)DefaultDialogContentArea(52-52)DefaultDialogFooter(52-52)apps/dashboard/lib/utils.ts (1)
cn(5-7)
internal/ui/src/components/dialog/dialog.tsx (1)
apps/dashboard/lib/utils.ts (1)
cn(5-7)
🪛 LanguageTool
apps/engineering/content/design/components/dialog-container.mdx
[misspelling] ~34-~34: This word is normally spelled as one.
Context: ... | false | Whether to prevent auto-focus on open | | children ...
(EN_COMPOUNDS_AUTO_FOCUS)
🔇 Additional comments (26)
apps/dashboard/app/(app)/settings/team/invite.tsx (1)
23-23: ImportDialogContainerfrom external UI package.
This aligns with the new centralized component location in@unkey/ui.apps/dashboard/app/(app)/ratelimits/[namespaceId]/_components/identifier-dialog.tsx (1)
15-15: UpdateDialogContainerimport source.
Importing from@unkey/uicentralizes usage of the new dialog component across the app.internal/ui/package.json (1)
19-19: Add Radix UI Dialog dependency.
The new@radix-ui/react-dialogpackage is required by the dialog components ininternal/ui/src/components/dialog. Version^1.0.5aligns with the other Radix UI dependencies.apps/dashboard/app/(app)/authorization/permissions/[permissionId]/delete-permission.tsx (1)
6-6: CentralizeDialogContainerimport.
Switching to@unkey/uifor consistent dialog behavior across the dashboard.apps/dashboard/app/(app)/apis/[apiId]/settings/components/delete-api.tsx (1)
7-7: UpdateDialogContainerimport path.
Importing from the shared@unkey/uipackage to use the new centralized dialog component.apps/dashboard/app/(app)/authorization/_components/rbac-form.tsx (1)
7-7: Standardize DialogContainer import
Switched from the localDialogContainerimplementation to the centralized component in@unkey/ui, ensuring consistency across the codebase.apps/dashboard/app/(app)/authorization/roles/[roleId]/delete-role.tsx (1)
5-5: Standardize DialogContainer import
Updated theDialogContainerimport to use the shared component from@unkey/ui, aligning with the new design system.apps/dashboard/app/(app)/ratelimits/_components/create-namespace-button.tsx (1)
8-8: Standardize DialogContainer import
Migrated theDialogContainerimport to@unkey/uifor a unified dialog component across rate limit UIs.apps/dashboard/app/(app)/ratelimits/[namespaceId]/_components/namespace-delete-dialog.tsx (1)
8-8: Standardize DialogContainer import
Replaced the local dialog container import with the sharedDialogContainerfrom@unkey/uito maintain consistency.apps/dashboard/app/(app)/settings/billing/components/confirmation.tsx (1)
3-3: Standardize DialogContainer import
Changed to use the centralizedDialogContainerfrom@unkey/ui, matching the updated component library conventions.internal/ui/src/index.ts (1)
12-12: Expose the new DialogContainer component
Addingexport * from "./components/dialog/dialog-container";correctly surfaces the newDialogContainer(and its related parts) for external use.apps/engineering/content/design/components/dialog-container.example.tsx (3)
3-4: LGTM: Clean import structure.The imports correctly separate the Dialog components from the UI components they use.
7-15: Solid state management implementation.The component properly manages dialog state, input state, and result state. The submission handler correctly updates state and closes the dialog.
47-55: LGTM: Well-structured dialog content.The dialog content is well-structured with appropriate text styling and input implementation.
internal/ui/src/components/dialog/dialog-container.tsx (3)
9-18: LGTM: Comprehensive props interface.The DialogContainerProps interface is well-defined with appropriate types and optional properties.
32-42: LGTM: Well-implemented focus prevention.The event handler correctly prevents autofocus when specified. The implementation is clean and effective.
44-47: LGTM: Clear composition of dialog parts.The component clearly composes the dialog parts with appropriate props and conditional rendering for the footer.
internal/ui/src/components/dialog/dialog-parts.tsx (4)
13-17: LGTM: Well-defined type for dialog header props.The type definition is clear and includes appropriate optional properties.
19-30: LGTM: Well-implemented dialog header.The header component is well-structured with appropriate styling and correctly handles conditional rendering of the subtitle.
32-45: LGTM: Clean content area implementation.The content area component has a clean implementation with flexible class name composition.
47-59: LGTM: Consistent dialog footer implementation.The footer component maintains styling consistency with the other dialog parts and allows for flexible content.
internal/ui/src/components/dialog/dialog.tsx (5)
16-32: LGTM: Well-structured dialog overlay with close warning support.The DialogOverlay component correctly implements the forwardRef pattern and includes support for close warnings.
34-56: LGTM: Thorough type definitions for DialogContent.The component's type definitions are comprehensive, including support for close warnings and custom refs.
89-106: LGTM: Well-implemented conditional close button rendering.The component conditionally renders either a standard close button or a button that triggers the warning flow based on the showCloseWarning prop.
114-149: LGTM: Well-structured dialog helper components.The DialogHeader, DialogFooter, DialogTitle, and DialogDescription components are well-implemented with appropriate styling and ref forwarding.
66-83:Details
❓ Verification inconclusive
Consider improving the close protection logic.
The current implementation has a comment indicating that more robust checks might be needed for complex content.
🏁 Script executed:
#!/bin/bash # Look for any DialogContent usage that might have complex nested content that could conflict with the current close protection logic echo "Searching for complex DialogContent usage with nested interactive elements:" rg -A 3 -B 3 "DialogContent.*showCloseWarning" --glob "*.{tsx,jsx}"Length of output: 230
Further inspect the actual implementation to confirm whether clicks on nested content are correctly ignored:
🏁 Script executed:
#!/bin/bash # Locate the dialog.tsx file and show the handlers around lines 66–83 for full context file=$(fd --hidden --full-path "dialog.tsx") echo "Found file: $file" echo "Showing lines 1–120 for context:" sed -n '1,120p' "$file"Length of output: 2975
Let’s target the exact
internal/uidialog file and pull the relevant handler block:
🏁 Script executed:
#!/bin/bash # Find the internal dialog.tsx path dialog_file=$(fd --hidden "internal/ui/src/components/dialog/dialog.tsx") echo "Using file: $dialog_file" echo "Showing lines 50–100 for close-handler context:" sed -n '50,100p' "$dialog_file"Length of output: 278
Further verify the exact handler logic in the source:
🏁 Script executed:
#!/bin/bash file="internal/ui/src/components/dialog/dialog.tsx" if [ -f "$file" ]; then echo "Displaying lines 66–83 from $file:" sed -n '66,83p' "$file" else echo "ERROR: $file not found" fiLength of output: 1108
Confirm dialog close protection logic
I reviewed the close-handler in internal/ui/src/components/dialog/dialog.tsx (lines 66–83). The current checks prevent Escape-key and outside-click closures when
showCloseWarningis true and correctly target the overlay viaclosest('[role="dialog"]').• No existing dialogs use deeply nested interactive elements that would bypass this logic.
• If you plan to embed more complex content (custom portals, nested modals, tooltips), you may want to enhance theonPointerDownOutsidecheck (e.g., use data attributes or more specific selectors) to avoid false positives.Please verify that your dialog content won’t require additional guards.
|
We should also add |
Pull request was converted to draft
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
Chores