fix(dashboard): hydration warnings#2253
Conversation
unrenamed
requested review from
chronark,
mcstepp and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@unrenamed is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily involve modifications to the Changes
Assessment against linked issues
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 (2)
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: 1
🧹 Outside diff range and nitpick comments (11)
apps/dashboard/app/(app)/user-button.tsx (1)
Line range hint
1-92: Overall component structure is solid, with a minor suggestion for accessibilityThe
UserButtoncomponent is well-structured and follows React best practices. The use of the "use client" directive is appropriate for this interactive component. The component handles different user states correctly and uses custom UI components effectively.To further improve accessibility, consider adding an
aria-labelto theDropdownMenuTriggerbutton to provide more context for screen readers.Here's a suggested minor improvement:
<DropdownMenuTrigger className="flex items-center justify-between gap-2 p-2 w-full h-12 rounded-[0.625rem] hover:bg-background-subtle hover:cursor-pointer text-content"> + aria-label="User menu" <div className="flex items-center gap-2 whitespace-nowrap overflow-hidden"> {/* ... existing content ... */} </div> </DropdownMenuTrigger>This change will provide better context for users relying on assistive technologies.
apps/dashboard/app/(app)/authorization/roles/[roleId]/delete-role.tsx (1)
71-71: Improved component composition withasChildpropThe addition of the
asChildprop toDialogTriggeris a positive change that aligns with the PR objectives. This modification:
- Improves the component structure by eliminating an unnecessary wrapper element.
- Enhances flexibility in composing the dialog trigger.
- Potentially resolves issues related to improper nesting of elements.
This change contributes to addressing the hydration warnings mentioned in the linked issue #2243 by ensuring a more predictable and semantically correct DOM structure.
Consider reviewing other similar component usages throughout the codebase for consistency. This pattern could be applied to other Radix UI components like
TooltipTriggerorPopoverTriggerif they're used elsewhere in the application.apps/dashboard/app/(app)/settings/billing/stripe/success/page.tsx (2)
57-66: Improved error message structure. Consider minor enhancement.The restructuring of the error message for a missing Stripe session significantly improves readability and user experience. The separation of the title and description, along with the use of the
Codecomponent for the session ID, makes the message clearer and easier to understand.Consider adding a brief explanation of what a Stripe session is or why it might be missing. This could help users better understand the context of the error. For example:
<EmptyPlaceholder.Description> - The Stripe session + The Stripe session (a temporary checkout session) </EmptyPlaceholder.Description> <Code>{session_id}</Code> <EmptyPlaceholder.Description> - you are trying to access does not exist. Please contact + you are trying to access does not exist or has expired. Please contact support@unkey.dev. </EmptyPlaceholder.Description>
Line range hint
1-150: Overall improvements align with PR objectives. Consider removing unnecessary type check.The changes made to the
StripeSuccessfunction successfully address the PR objective of improving error message clarity. The restructured error messages for both Stripe session and customer not found scenarios enhance readability and user experience.Based on the retrieved learning, TypeScript's type-checking ensures that
new_planonly accepts "free", "pro", or undefined values. Therefore, the runtime check in theisUpgradingPlanvariable assignment might be redundant. Consider simplifying it:- const isUpgradingPlan = new_plan && new_plan !== ws.plan && new_plan === "pro"; + const isUpgradingPlan = new_plan === "pro" && new_plan !== ws.plan;This change maintains the same logic while relying on TypeScript's type safety, potentially improving code readability.
apps/dashboard/app/(app)/settings/vercel/page.tsx (1)
70-70: Approved: Enhanced error message formattingThe addition of JSON formatting for the error message is a good improvement. It enhances readability and aids in debugging, which aligns with the PR objective of improving error message clarity.
Consider adding a character limit to the error message to prevent potentially large error displays:
- <Code className="text-left">{JSON.stringify(err, null, 2)}</Code> + <Code className="text-left">{JSON.stringify(err, null, 2).slice(0, 500)}{JSON.stringify(err, null, 2).length > 500 ? '...' : ''}</Code>This change would limit the error message to 500 characters and add an ellipsis if it's truncated, balancing between providing useful information and preventing overly large error displays.
apps/dashboard/app/(app)/settings/root-keys/[keyId]/permissions/permission-manager-card.tsx (2)
Line range hint
72-72: Good addition of key prop, consider using a more unique identifierAdding the
key={action}prop to the PermissionToggle component is a good step towards preventing hydration warnings and improving rendering performance. This change aligns well with the PR objectives.However, consider using a more unique identifier if available, such as
${category}-${action}or a dedicated id field if one exists. This would ensure uniqueness across all categories, not just within each category.If a more unique identifier is available, consider updating the key prop:
- key={action} + key={`${category}-${action}`}
99-99: Good addition of key props, consider consistent key structureAdding
key={category}andkey={action}props to the outer and inner divs in the activePermissions mapping is a significant improvement. This change addresses potential hydration warnings and aligns with the PR objectives.For consistency with the earlier suggestion, consider using a compound key for the inner div to ensure uniqueness across all categories.
Consider updating the inner div key prop for consistency:
- <div key={action} className="flex items-center gap-8"> + <div key={`${category}-${action}`} className="flex items-center gap-8">This change would make the key structure consistent throughout the component and ensure uniqueness across all categories.
Also applies to: 108-108
apps/dashboard/app/(app)/authorization/roles/create-new-role.tsx (1)
98-98: Excellent use ofasChildprop to improve component structureThe addition of the
asChildprop to theDialogTriggercomponent is a positive change that aligns well with the PR objectives. This modification:
- Helps resolve potential hydration warnings by ensuring consistent DOM structure between server and client rendering.
- Improves accessibility by removing unnecessary DOM nesting.
- Enhances component composability, following Radix UI best practices.
This change effectively addresses the structural HTML warnings mentioned in the PR objectives and contributes to a more robust component architecture.
Consider reviewing other similar component usages throughout the codebase to ensure consistent application of this pattern, which could further reduce hydration warnings and improve overall application structure.
apps/dashboard/app/(app)/team-switcher.tsx (1)
Line range hint
83-87: Approved: Good use ofasChildprop to improve component structure.The introduction of the
asChildprop on theTooltipTriggercomponent is a positive change. This modification addresses potential hydration warnings by allowing the childspanelement to act as the trigger, which aligns well with the PR objectives.To further enhance accessibility, consider adding an
aria-labelto thespanelement:<TooltipTrigger asChild> - <span className="overflow-hidden text-sm font-medium text-ellipsis"> + <span className="overflow-hidden text-sm font-medium text-ellipsis" aria-label="Current workspace"> {currentOrg?.name ?? "Personal Workspace"} </span> </TooltipTrigger>This addition will provide more context for screen reader users.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/page.tsx (2)
276-279: Consistent UI improvement, but consider refactoringThe addition of the
Codecomponent with thesnippetandCopyButtonin this section maintains consistency with the earlier "Requests" section, which is great for user experience. However, this introduces code duplication.Consider refactoring this repeated code block into a separate component to improve maintainability. For example:
const EmptyStateCodeSnippet = ({ snippet }: { snippet: string }) => ( <Code className="flex items-start gap-8 p-4 my-8 text-xs text-left"> {snippet} <CopyButton value={snippet} /> </Code> );Then, you can use this component in both
EmptyPlaceholderinstances:<EmptyPlaceholder> {/* ... other EmptyPlaceholder content ... */} <EmptyStateCodeSnippet snippet={snippet} /> </EmptyPlaceholder>This refactoring would reduce duplication and make future updates to this UI element easier to manage.
Line range hint
1-379: Summary: Improved empty state UI with consistent code snippetsThe changes in this file effectively address the PR objectives by enhancing the empty state UI for both the "Requests" and "Top identifiers" sections. The addition of code snippets with copy functionality provides valuable information to users when there's no data to display, which aligns well with the goal of improving user experience and addressing UI issues.
Key points:
- Consistent improvement across both empty state sections.
- No changes to core logic or data fetching, maintaining existing functionality.
- No new warnings or errors introduced.
While the changes are positive, consider the earlier suggestion to refactor the repeated code snippet into a separate component. This would enhance maintainability and adhere to the DRY (Don't Repeat Yourself) principle, which is particularly important as the codebase grows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
- apps/dashboard/app/(app)/authorization/permissions/[permissionId]/delete-permission.tsx (1 hunks)
- apps/dashboard/app/(app)/authorization/permissions/[permissionId]/page.tsx (1 hunks)
- apps/dashboard/app/(app)/authorization/permissions/create-new-permission.tsx (1 hunks)
- apps/dashboard/app/(app)/authorization/roles/[roleId]/delete-role.tsx (1 hunks)
- apps/dashboard/app/(app)/authorization/roles/[roleId]/update-role.tsx (1 hunks)
- apps/dashboard/app/(app)/authorization/roles/create-new-role.tsx (1 hunks)
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/page.tsx (2 hunks)
- apps/dashboard/app/(app)/ratelimits/sparkline.tsx (1 hunks)
- apps/dashboard/app/(app)/semantic-cache/[gatewayId]/analytics/page.tsx (2 hunks)
- apps/dashboard/app/(app)/settings/billing/plans/button.tsx (1 hunks)
- apps/dashboard/app/(app)/settings/billing/stripe/success/page.tsx (2 hunks)
- apps/dashboard/app/(app)/settings/root-keys/[keyId]/permissions/permission-manager-card.tsx (4 hunks)
- apps/dashboard/app/(app)/settings/vercel/page.tsx (1 hunks)
- apps/dashboard/app/(app)/team-switcher.tsx (1 hunks)
- apps/dashboard/app/(app)/user-button.tsx (1 hunks)
- apps/dashboard/components/dashboard/empty-placeholder.tsx (1 hunks)
🧰 Additional context used
📓 Learnings (1)
apps/dashboard/app/(app)/settings/billing/stripe/success/page.tsx (1)
Learnt from: mcstepp PR: unkeyed/unkey#2120 File: apps/dashboard/app/(app)/settings/billing/stripe/success/page.tsx:19-19 Timestamp: 2024-09-20T18:32:21.963Z Learning: In the `StripeSuccess` function, TypeScript's type-checking of the `new_plan` parameter ensures that only "free", "pro", or undefined values are accepted, so additional runtime validation is not necessary.
🔇 Additional comments (16)
apps/dashboard/app/(app)/ratelimits/sparkline.tsx (1)
15-15: Correct removal ofasynckeywordThe removal of the
asynckeyword from theSparklinecomponent's function signature is appropriate and beneficial. This change:
- Aligns the component with its synchronous nature, as there are no asynchronous operations within it.
- Addresses the PR objective of fixing "unsupported async/await functionality in client components".
- Helps reduce potential hydration warnings and rendering issues.
The rest of the component's logic remains intact, ensuring that its functionality is preserved while improving its structure.
apps/dashboard/components/dashboard/empty-placeholder.tsx (1)
51-51: Confirm the intended margin reduction for EmptyPlaceholder.DescriptionThe margin-bottom for the description has been reduced from
mb-8tomb-2. This change will decrease the vertical spacing below the description text.While this modification doesn't directly address hydration warnings (the main focus of the PR), it does align with the PR objective of adjusting margins to improve the user interface. However, it's important to ensure that this change doesn't negatively impact the overall layout or readability of the dashboard.
To verify the impact of this change:
- Please provide before and after screenshots of the EmptyPlaceholder component in various contexts where it's used.
- Confirm that this margin adjustment is intentional and not an accidental modification.
Consider adding a comment explaining the reason for this specific margin value, which could help prevent unintended changes in the future.
apps/dashboard/app/(app)/user-button.tsx (1)
Line range hint
38-43: Improved component structure withasChildpropThe changes to the
TooltipTriggercomponent are well-implemented and address the PR objectives. By using theasChildprop, you've improved the HTML structure and potentially resolved hydration warnings related to improper nesting.This modification allows the
TooltipTriggerto render its childspanelement directly, which can lead to a more predictable DOM structure during client-side rendering. This aligns well with the goal of preventing hydration mismatches between server and client rendering.apps/dashboard/app/(app)/settings/billing/plans/button.tsx (1)
63-63: Approved: Good use ofasChildto address nesting issues.The addition of the
asChildprop to theDialogTriggercomponent is a positive change that aligns with the PR objectives. It helps address the improper nesting of<button>elements and potentially resolves hydration warnings.Please verify the following:
- Ensure that the
disabledprop onDialogTriggerstill functions correctly when used withasChild.- Test the component thoroughly to confirm that this change doesn't introduce any new UI/UX issues.
To assist with verification, you can run the following script:
This will help identify other instances of
DialogTriggerthat might benefit from theasChildprop, ensuring consistency across the codebase.apps/dashboard/app/(app)/authorization/permissions/[permissionId]/delete-permission.tsx (1)
70-70: Approved: Improved DialogTrigger implementationThe addition of the
asChildprop toDialogTriggeris a positive change. This modification allows the trigger element to be rendered directly without an additional wrapper, which can help resolve potential HTML structure warnings and improve overall component composition.This change:
- Enhances flexibility in styling and behavior of the trigger element.
- Aligns with Radix UI best practices.
- Contributes to addressing the hydration warnings mentioned in the PR objectives.
The core functionality of the
DeletePermissioncomponent remains intact, making this a safe and beneficial improvement.apps/dashboard/app/(app)/authorization/permissions/create-new-permission.tsx (1)
78-78: Approved: Addition ofasChildprop resolves potential button nesting issues.The addition of the
asChildprop toDialogTriggeris a good solution to address the improper nesting of<button>elements, which aligns with the PR objectives and helps resolve hydration warnings.To ensure this change doesn't introduce any accessibility issues, please verify that the
triggerprop passed toCreateNewPermissionis always an interactive element (e.g., a button or anchor tag). You can use the following script to check the usage ofCreateNewPermission:This script will help identify any instances where
CreateNewPermissionmight be used with a non-interactive trigger element.✅ Verification successful
Verified: All usages of
CreateNewPermissionutilize interactive elements for thetriggerprop.The addition of the
asChildprop toDialogTriggerhas been correctly implemented, ensuring accessibility and preventing hydration warnings without introducing any issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of CreateNewPermission component # Search for CreateNewPermission usage echo "Searching for CreateNewPermission usage:" rg --type typescript --type typescriptreact 'CreateNewPermission' -C 3 # Search for potential non-interactive triggers echo "\nChecking for potential non-interactive triggers:" rg --type typescript --type typescriptreact 'CreateNewPermission.*trigger=\{[^}]*<div' -C 3 rg --type typescript --type typescriptreact 'CreateNewPermission.*trigger=\{[^}]*<span' -C 3Length of output: 566
Script:
#!/bin/bash # Description: Check usage of CreateNewPermission component # Search for CreateNewPermission usage in .ts and .tsx files echo "Searching for CreateNewPermission usage in .ts and .tsx files:" rg 'CreateNewPermission' -g '*.ts' -g '*.tsx' -C 3 # Search for potential non-interactive triggers in .ts and .tsx files echo -e "\nChecking for potential non-interactive triggers in .ts and .tsx files:" rg 'CreateNewPermission.*trigger=\{[^}]*<div' -g '*.ts' -g '*.tsx' -C 3 rg 'CreateNewPermission.*trigger=\{[^}]*<span' -g '*.ts' -g '*.tsx' -C 3Length of output: 5280
apps/dashboard/app/(app)/authorization/roles/[roleId]/update-role.tsx (1)
77-77: Approved: Improved DialogTrigger renderingThe addition of the
asChildprop to theDialogTriggercomponent is a positive change. This modification allows thetriggercontent to be rendered directly as a child ofDialogTriggerwithout an additional wrapping element.This change aligns well with the PR objectives of addressing structural HTML warnings and improving component rendering. It may help resolve issues related to improper nesting of elements, which was mentioned in the linked issue #2243.
The functionality of the
DialogTriggerremains intact, and this change follows modern React patterns and best practices for using Radix UI components.apps/dashboard/app/(app)/settings/vercel/page.tsx (1)
Line range hint
1-146: Overall assessment: Positive improvementThe change in this file successfully addresses one of the PR objectives by improving error message clarity. The modification is minimal yet impactful, enhancing the developer experience without introducing new issues or altering the core functionality of the component.
The rest of the file, including the logic for fetching workspace data, handling integrations, and processing Vercel projects, remains intact. This ensures that the existing functionality is preserved while the error handling is improved.
apps/dashboard/app/(app)/authorization/permissions/[permissionId]/page.tsx (1)
87-90: Improved tooltip structure and layoutThe changes to the
TooltipTriggercomponent are well-implemented and address several important aspects:
- The use of the
asChildprop onTooltipTriggerallows for more flexible rendering of its children, which can help resolve potential hydration issues.- Wrapping the content in a
divwith flex layout improves the structure and alignment of the tooltip content.- The
truncateclass on the permission name ensures that long names are displayed properly without breaking the layout.- Wrapping the
CopyButtonin adivprovides better control over its positioning within the layout.These changes align well with the PR objectives of addressing structural HTML warnings and improving component structure. They should contribute to resolving hydration issues and enhancing the overall stability of the application.
apps/dashboard/app/(app)/settings/root-keys/[keyId]/permissions/permission-manager-card.tsx (3)
15-15: LGTM: Import changes improve code cleanlinessThe addition of the
Checkimport and removal of the unusedPencilimport enhance code cleanliness. This change aligns with best practices for maintaining a clean and efficient codebase.
65-65: Excellent: Added key prop to improve list renderingAdding the
key={category}prop to the outer div in the mapping ofprops.permissionsStructureis a crucial improvement. This change helps React's reconciliation process and can prevent hydration warnings related to list rendering. It's an important step in addressing the issues mentioned in the PR objectives.
Line range hint
1-132: Overall: Good progress on addressing hydration warningsThe changes made in this file are a significant step towards addressing the hydration warnings mentioned in the PR objectives. The addition of
keyprops to mapped elements in various parts of thePermissionManagerCardcomponent will help prevent rendering issues and improve React's reconciliation process.Key improvements:
- Added
keyprops to all mapped elements.- Removed unused imports, improving code cleanliness.
While these changes are positive, consider the suggestions for using more unique and consistent key structures throughout the component. This would further enhance the robustness of the rendering process.
No major issues were identified, and the changes align well with the PR objectives. Great job on improving the component structure and addressing potential hydration warnings!
apps/dashboard/app/(app)/semantic-cache/[gatewayId]/analytics/page.tsx (3)
9-9: Improved import specificityThe change to import only
getSemanticCachesDailyandgetSemanticCachesHourlyis a good optimization. It aligns with the PR objectives by potentially reducing unnecessary code and improving performance.
220-223: Enhanced code snippet presentationThe changes to the Code component within EmptyPlaceholder improve the layout and readability of the code snippet. The added gap between the snippet and the copy button enhances usability. These modifications align with the PR objectives by improving the user interface.
Line range hint
1-243: Overall improvements to code organization and UIThe changes in this file effectively address the PR objectives by:
- Streamlining imports to focus on necessary functions, potentially improving performance.
- Enhancing the user interface for the code snippet display.
These modifications contribute to resolving hydration warnings and improve the overall code quality and user experience. No new issues or potential problems were introduced.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/page.tsx (1)
232-235: Improved UI with code snippet and copy functionalityThe addition of the
Codecomponent with thesnippetandCopyButtonenhances the user experience when there's no usage data. This change provides valuable information to users and allows for easy copying of the samplecurlcommand, which aligns well with the PR objectives of improving the UI and addressing structural warnings.
apps/dashboard/app/(app)/settings/billing/stripe/success/page.tsx
Outdated
Show resolved
Hide resolved
|
f2fcbd6 adjusts margin for While refactoring this wasn't the focus of this PR, I had to reduce the margin to fix the UI after addressing the warnings. From my perspective, the updated UI now looks more consistent. It’s something to review carefully, as it may influence layout in other parts of the application where this component is used. |
|
Awarding unrenamed: 150 points 🕹️ Well done! Check out your new contribution on oss.gg/unrenamed |
2 participants
What does this PR do?
Resolves structural HTML warnings, such as improper nesting of
<button>elements and<prev>inside<p>, adjusts the margins for theEmptyPlaceholder.Descriptioncomponent to enhance the UI, fixes an issue with unsupported async/await functionality in client components, and ensures that each child in arrays is assigned a unique "key" prop to prevent rendering errors.Fixes #2243
This PR does not resolve all the warnings mentioned above. There may still be instances where these warnings could arise, especially due to the complexity of the various UI component renderings and business logic combinations that I'm not fully familiar with.
I have addressed the majority of the warnings that I was able to detect during the code analysis and live coding sessions, achieving the initial goal set for this PR.
🎯 The goal
The primary goal of this PR is to bring attention to potential issues that may cause unpredictable behavior during client-side rendering in the future (see #2243 for more details). This should help the repository maintainers determine whether to address these concerns during code reviews or consider enhancing the development and review processes to prevent such warnings from reaching production.
Type of change
How should this be tested?
In HTML, <button> cannot be a descendant of <button>. This will cause a hydration error.In HTML, <pre> cannot be a descendant of <p>. This will cause a hydration error.async/await is not yet supported in Client Components, only Server Components. This error is often caused by accidentally adding 'use client' to a module that was originally written for the server.Each child in an array or iterator should have a unique "key" prop.Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
Release Notes
New Features
asChildprop, improving interaction and rendering.DialogTriggercomponents for better user experience.Bug Fixes
Style
Chores