chore: dialog loading improvements - Merge after #3280#3281
chore: dialog loading improvements - Merge after #3280#3281ogzhanolguncu merged 4 commits intomainfrom
Conversation
ogzhanolguncu
requested review from
chronark,
mcstepp and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
📝 WalkthroughWalkthroughThis change introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ApisNavbar
participant CreateKeyDialog
participant RBACDialogContent
participant DB
User->>ApisNavbar: Open Create Key Dialog
ApisNavbar->>CreateKeyDialog: Pass keyspaceDefaults prop
CreateKeyDialog->>CreateKeyDialog: Initialize form with getDefaultValues(keyspaceDefaults)
Note over CreateKeyDialog: Form resets if keyspaceDefaults change
User->>ApisNavbar: Open Permissions Dialog
ApisNavbar->>RBACDialogContent: Render with keyId, keyspaceId
RBACDialogContent->>DB: Fetch permissions via TRPC
RBACDialogContent-->>ApisNavbar: Display RBAC info
Suggested labels
Suggested reviewers
✨ 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! 🙏 |
![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 (4)
apps/dashboard/app/(app)/apis/[apiId]/api-id-navbar.tsx (1)
14-61: Well-implemented dynamic imports with appropriate loading states.The dynamic imports correctly disable SSR and provide loading fallbacks for user-facing components. This aligns with the PR objective of reducing initial bundle size.
Consider adding a loading fallback for
DialogContainerto ensure consistent loading behavior:const DialogContainer = dynamic( () => import("@/components/dialog-container").then((mod) => mod.DialogContainer), { ssr: false, + loading: () => <div className="fixed inset-0 bg-black/50" />, }, );apps/dashboard/app/(app)/apis/[apiId]/_components/rbac-dialog-content.tsx (3)
9-23: Consider adding loading fallbacks for dynamic components.For consistency with other dynamic imports and better UX, consider adding loading states:
const PermissionList = dynamic( () => import("../keys/[keyAuthId]/[keyId]/components/rbac/permissions").then((mod) => ({ default: mod.PermissionList, })), - { ssr: false }, + { + ssr: false, + loading: () => <div className="animate-pulse h-32 bg-gray-100 rounded" /> + }, ); const RBACButtons = dynamic( () => import("../keys/[keyAuthId]/[keyId]/components/rbac/rbac-buttons").then((mod) => ({ default: mod.RBACButtons, })), - { ssr: false }, + { + ssr: false, + loading: () => <div className="animate-pulse h-10 w-24 bg-gray-100 rounded" /> + }, );
53-80: Robust error handling with retry capability.The implementation provides clear error messaging and proper retry functionality through cache invalidation.
Consider displaying more specific error messages based on the error type:
- <div className="text-accent-10 text-sm">Could not retrieve permission data</div> + <div className="text-accent-10 text-sm"> + {error?.message || "Could not retrieve permission data"} + </div>
119-155: Consider improving type safety in the helper function.The logic is correct, but using
anytypes reduces type safety:- const roles = permissionsData.workspace.roles.map((role: any) => { + const roles = permissionsData.workspace.roles.map((role) => { return { id: role.id, name: role.name, - isActive: permissionsData.roles.some((keyRole: any) => keyRole.roleId === role.id), + isActive: permissionsData.roles.some((keyRole) => keyRole.roleId === role.id), }; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/create-key.utils.ts(4 hunks)apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/index.tsx(4 hunks)apps/dashboard/app/(app)/apis/[apiId]/_components/rbac-dialog-content.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/actions.ts(3 hunks)apps/dashboard/app/(app)/apis/[apiId]/api-id-navbar.tsx(4 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/keys-list.tsx(3 hunks)apps/dashboard/app/(app)/apis/[apiId]/settings/components/default-bytes.tsx(3 hunks)apps/dashboard/app/(app)/apis/[apiId]/settings/components/default-prefix.tsx(3 hunks)apps/dashboard/app/(app)/apis/[apiId]/settings/components/settings-client.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/settings/page.tsx(1 hunks)apps/dashboard/lib/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/keys-list.tsx (3)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/keys-table-action.popover.tsx (1)
KeysTableActionPopover(27-138)apps/dashboard/lib/utils.ts (1)
cn(5-7)internal/icons/src/icons/dots.tsx (1)
Dots(15-60)
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/index.tsx (1)
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/create-key.utils.ts (1)
getDefaultValues(89-125)
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/create-key.utils.ts (2)
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/create-key.schema.ts (1)
FormValueTypes(337-359)apps/dashboard/lib/utils.ts (1)
deepMerge(187-209)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (23)
apps/dashboard/app/(app)/apis/[apiId]/settings/components/settings-client.tsx (1)
31-32: LGTM!Properly passing the
apiIdprop to child components to enable revalidation after mutations. This is a clean implementation that maintains component separation of concerns.apps/dashboard/app/(app)/apis/[apiId]/settings/components/default-bytes.tsx (4)
2-2: LGTM!Proper import of the
revalidatefunction to enable cache invalidation after mutations.
25-25: LGTM!Correctly added the
apiIdprop to the Props type definition.
28-28: LGTM!Properly updated the component function signature to accept the
apiIdprop.
61-61: LGTM!Excellent placement of the
revalidatecall after successful mutation to ensure the UI reflects the updated settings immediately.apps/dashboard/app/(app)/apis/[apiId]/settings/components/default-prefix.tsx (4)
2-2: LGTM!Proper import of the
revalidatefunction, consistent with theDefaultBytescomponent.
27-27: LGTM!Correctly added the
apiIdprop to the Props type definition, maintaining consistency with theDefaultBytescomponent.
30-30: LGTM!Properly updated the component function signature to accept the
apiIdprop.
61-61: LGTM!Excellent placement of the
revalidatecall after successful mutation, ensuring immediate UI updates and consistency with theDefaultBytescomponent implementation.apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/keys-list.tsx (2)
5-5: Good addition of the Dots icon import.The
Dotsicon is correctly imported to support the loading fallback component.
37-56: Excellent implementation of lazy loading with consistent styling.The dynamic import implementation follows Next.js best practices:
- SSR is properly disabled for client-side components
- Loading fallback maintains visual consistency with the actual component
- The styling in the fallback matches the actual component's button styling from the relevant code snippets
This change effectively reduces the initial bundle size by deferring the
KeysTableActionPopovercomponent load until user interaction.apps/dashboard/app/(app)/apis/[apiId]/settings/page.tsx (1)
42-51: Well-structured data transformation for keyspace defaults.The
currentApiobject construction properly extracts and reshapes the necessary properties. The use of logical OR (||) withundefinedensures consistent nullish values instead of mixed falsy values, which is good for type safety downstream.This transformation aligns with the API interface expected by
ApisNavbarand enables the keyspace defaults to be passed through the component hierarchy.apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/index.tsx (3)
36-44: Proper prop type definition for keyspace defaults.The
keyspaceDefaultsprop is well-defined with optionalprefixandbytesproperties, allowing for flexible default value configuration.
61-61: Correct integration of keyspace defaults in form initialization.The form now properly uses
keyspaceDefaultsfor initial values, ensuring the form reflects the keyspace configuration from the database.
77-82: Excellent form synchronization logic.The
useEffectproperly handles form state updates whenkeyspaceDefaultschange after revalidation:
- Clears persisted data to prevent stale state
- Resets form with new default values
- Has correct dependencies to avoid unnecessary re-renders
This ensures the form always reflects the current keyspace defaults, improving user experience.
apps/dashboard/app/(app)/apis/[apiId]/actions.ts (3)
14-17: Well-defined type extension for keyspace defaults.The
keyspaceDefaultsproperty is properly typed with optionalprefixandbytesfields, maintaining type safety while allowing for flexible configuration.
41-46: Efficient database query modification.The
keyAuthrelation is added with selective column fetching (defaultPrefixanddefaultBytes), which is efficient as it only retrieves the necessary data for keyspace defaults.
75-78: Proper data mapping with consistent null handling.The
keyspaceDefaultsobject construction correctly maps database values and uses logical OR withundefinedto ensure consistent nullish value handling, which aligns with the type definition and downstream usage.apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/create-key.utils.ts (2)
89-91: LGTM! Well-designed function signature.The optional
overridesparameter withPartial<FormValueTypes> | nulltype allows for flexible default value customization while maintaining backward compatibility.
103-103: Good type safety improvement.Using
as constensures TypeScript treats this as a literal type, providing better type safety when working with union types.apps/dashboard/app/(app)/apis/[apiId]/api-id-navbar.tsx (2)
73-77: Good type definition for keyspaceDefaults.The nullable object with optional properties provides flexibility for API-specific default configurations.
196-206: Clean implementation of RBAC dialog.The modular approach with
RBACDialogContenthandling all permissions logic improves code organization and maintainability.apps/dashboard/app/(app)/apis/[apiId]/_components/rbac-dialog-content.tsx (1)
30-106: Well-structured component with proper state handling.The component correctly handles all states (loading, error, success) and provides a good user experience with retry functionality and appropriate loading indicators.
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/create-key.utils.ts
Show resolved
Hide resolved
|
@ogzhanolguncu there's a conflict |
|
This one is good to go. @MichaelUnkey |
MichaelUnkey
left a comment
MichaelUnkey
left a comment
There was a problem hiding this comment.
It looks good. Just have a couple questions.
- Is there a purpose to having the "Add Permission" button. You can add a permission but it does not get added to the key.
- Should we be able to see permissions in the Dialog and add them?
|
@MichaelUnkey Where is that "Add Permission" :thinkies:. Permission section is a temporary solution till we refactor it with new RBAC design. This PRs goal is to reduce the bundle if possible and make it a tiny bit faster by dynamically loading |
4 participants
What does this PR do?
Reduced initial bundle size by lazy-loading heavy components that aren't always needed.
Changes
CreateKeyDialog- Only loads when user has keyspace accessKeysTableActionPopover- Loads per table row interaction, reducing table overheadDialogContainer&RBACDialogContent- Load only when dialogs are openedFixes # (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
Bug Fixes