chore: Navigation becomes loosely tied to the pages#2927
Conversation
This breaks navigation to a simple navigation.tsx file allowing for full control on each page or segment, with the ability to make single changes without breaking entire layouts.
Refactors for a generic navigation and finishes the final ones
perkinsjr
requested review from
chronark,
mcstepp and
ogzhanolguncu
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
📝 WalkthroughWalkthroughThe changes replace the legacy Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant N as Navigation Component
participant NB as Navbar UI Layer
U->>N: Load page with navigation props
N->>NB: Render breadcrumbs and action buttons
U->>NB: Click on a breadcrumb or action (e.g., copy ID, create key)
NB->>N: Handle the user interaction event
N-->>U: Trigger navigation or copy action feedback
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 (17)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
apps/dashboard/app/(app)/settings/root-keys/navigation.tsx (2)
8-23: Breadcrumb text doesn't fully match the navigation context.The breadcrumb text is simply "Settings" but the href is "/settings/root-keys". Consider changing the text to be more specific (e.g., "Root Keys") to better represent where the user will navigate to, or add another breadcrumb to show the full navigation path (Settings > Root Keys).
11-15: Consider adding a more descriptive breadcrumb hierarchy.For better user navigation context, consider adding a parent breadcrumb for "Settings" that leads to the main settings page, followed by the "Root Keys" as the active page:
<Navbar.Breadcrumbs icon={<Gear />}> + <Navbar.Breadcrumbs.Link href="/settings">Settings</Navbar.Breadcrumbs.Link> - <Navbar.Breadcrumbs.Link href="/settings/root-keys" active> - Settings + <Navbar.Breadcrumbs.Link href="/settings/root-keys" active> + Root Keys </Navbar.Breadcrumbs.Link> </Navbar.Breadcrumbs>apps/dashboard/app/(app)/authorization/roles/[roleId]/navigation.tsx (1)
13-19: Improve breadcrumb hierarchy clarity.The first two breadcrumbs both say "Authorization" and "Roles" but have the same href. Consider changing the first breadcrumb to just link to the authorization page:
<Navbar.Breadcrumbs icon={<ShieldKey />}> - <Navbar.Breadcrumbs.Link href="/authorization/roles">Authorization</Navbar.Breadcrumbs.Link> + <Navbar.Breadcrumbs.Link href="/authorization">Authorization</Navbar.Breadcrumbs.Link> <Navbar.Breadcrumbs.Link href="/authorization/roles">Roles</Navbar.Breadcrumbs.Link> <Navbar.Breadcrumbs.Link href={`/authorization/roles/${role.id}`} isIdentifier active> {role.id} </Navbar.Breadcrumbs.Link> </Navbar.Breadcrumbs>apps/dashboard/app/(app)/apis/[apiId]/settings/navigation.tsx (1)
23-31: Consider adding a more descriptive label for the API ID badge.The badge shows only the raw ID without context. Consider adding a small label for clarity:
<Badge key="apiId" variant="secondary" className="flex justify-between w-full gap-2 font-mono font-medium ph-no-capture" > + <span className="text-muted-foreground">API ID:</span> {api.id} <CopyButton value={api.id} /> </Badge>apps/dashboard/components/navigation/navigation.tsx (1)
7-7: Consider adding optional props for flexibility.To make this component even more reusable, you might want to add optional props for additional actions or multiple breadcrumbs:
export function Navigation({ href, name, icon, + actions, + parentBreadcrumbs }: { href: string; name: string; icon: ReactNode; + actions?: ReactNode; + parentBreadcrumbs?: Array<{href: string; name: string}>; }) {This would allow the component to handle more complex navigation scenarios while maintaining the same streamlined API for simple cases.
apps/dashboard/app/(app)/settings/team/page.tsx (1)
104-104: Consider standardizing prop ordering for consistency.The order of props differs from line 54 (
href,name,icon) to line 104 (href,icon,name). While this doesn't affect functionality, consistent ordering improves readability and maintainability.- <Navigation href="/settings/team" icon={<Gear />} name="Settings" /> + <Navigation href="/settings/team" name="Settings" icon={<Gear />} />apps/dashboard/app/(app)/authorization/roles/navigation.tsx (2)
8-25: Consider strengthening the type definitions.The interface uses
anytypes for permissions which could be more strongly typed for better type safety.- permissions: Array<{ - permission: any; // We could type this further if needed - }>; + permissions: Array<{ + permission: { + id: string; + name: string; + description?: string | null; + }; + }>; - permissions: Array<any>; // We need this for CreateNewRole component + permissions: Array<{ + id: string; + name: string; + description?: string | null; + }>; // We need this for CreateNewRole component
37-40: Consider making the infinity limit more explicit.Using
Number.POSITIVE_INFINITYworks but may be confusing in the UI when formatted.- {Intl.NumberFormat().format(Number.POSITIVE_INFINITY)} used{" "} + {Intl.NumberFormat().format(Number.POSITIVE_INFINITY)} used (unlimited){" "}apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/navigation.tsx (1)
42-43: Consider making the badge width more constrained.The
w-fullclass on the Badge might make it unnecessarily wide in some layouts.- className="flex justify-between w-full gap-2 font-mono font-medium ph-no-capture" + className="flex justify-between max-w-md gap-2 font-mono font-medium ph-no-capture"apps/dashboard/app/(app)/authorization/permissions/[permissionId]/navigation.tsx (1)
14-14: Consider extracting the tooltip threshold to a constant.The magic number
16used for determining tooltip display could be extracted to a named constant at the top of the file for better maintainability.+const PERMISSION_NAME_TOOLTIP_THRESHOLD = 16; export function Navigation({ permissionId, permission, }: { permissionId: string; permission: Permission }) { - const shouldShowTooltip = permission.name.length > 16; + const shouldShowTooltip = permission.name.length > PERMISSION_NAME_TOOLTIP_THRESHOLD;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/navigation.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/page.tsx(2 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/navigation.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/page.tsx(2 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/navigation.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/navigation.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/page.tsx(2 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/page.tsx(2 hunks)apps/dashboard/app/(app)/apis/[apiId]/navigation.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/page.tsx(2 hunks)apps/dashboard/app/(app)/apis/[apiId]/settings/navigation.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/settings/page.tsx(2 hunks)apps/dashboard/app/(app)/apis/navigation.tsx(1 hunks)apps/dashboard/app/(app)/apis/page.tsx(2 hunks)apps/dashboard/app/(app)/audit/navigation.tsx(1 hunks)apps/dashboard/app/(app)/audit/page.tsx(2 hunks)apps/dashboard/app/(app)/authorization/permissions/[permissionId]/navigation.tsx(1 hunks)apps/dashboard/app/(app)/authorization/permissions/[permissionId]/page.tsx(2 hunks)apps/dashboard/app/(app)/authorization/permissions/navigation.tsx(1 hunks)apps/dashboard/app/(app)/authorization/permissions/page.tsx(2 hunks)apps/dashboard/app/(app)/authorization/roles/[roleId]/navigation.tsx(1 hunks)apps/dashboard/app/(app)/authorization/roles/[roleId]/page.tsx(2 hunks)apps/dashboard/app/(app)/authorization/roles/navigation.tsx(1 hunks)apps/dashboard/app/(app)/authorization/roles/page.tsx(2 hunks)apps/dashboard/app/(app)/identities/[identityId]/navigation.tsx(1 hunks)apps/dashboard/app/(app)/identities/[identityId]/page.tsx(2 hunks)apps/dashboard/app/(app)/identities/navigation.tsx(1 hunks)apps/dashboard/app/(app)/identities/page.tsx(2 hunks)apps/dashboard/app/(app)/logs/navigation.tsx(1 hunks)apps/dashboard/app/(app)/logs/page.tsx(2 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/namespace-navbar.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/navigation.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/page.tsx(2 hunks)apps/dashboard/app/(app)/settings/billing/page.tsx(2 hunks)apps/dashboard/app/(app)/settings/billing/plans/navigation.tsx(1 hunks)apps/dashboard/app/(app)/settings/billing/plans/page.tsx(2 hunks)apps/dashboard/app/(app)/settings/general/page.tsx(2 hunks)apps/dashboard/app/(app)/settings/root-keys/[keyId]/navigation.tsx(1 hunks)apps/dashboard/app/(app)/settings/root-keys/[keyId]/page.tsx(2 hunks)apps/dashboard/app/(app)/settings/root-keys/navigation.tsx(1 hunks)apps/dashboard/app/(app)/settings/root-keys/new/page.tsx(2 hunks)apps/dashboard/app/(app)/settings/root-keys/page.tsx(2 hunks)apps/dashboard/app/(app)/settings/team/page.tsx(3 hunks)apps/dashboard/app/(app)/settings/user/page.tsx(2 hunks)apps/dashboard/app/(app)/settings/vercel/page.tsx(3 hunks)apps/dashboard/components/navigation/navigation.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/namespace-navbar.tsx
🧰 Additional context used
🧠 Learnings (1)
apps/dashboard/app/(app)/authorization/roles/[roleId]/page.tsx (1)
Learnt from: AkshayBandi027
PR: unkeyed/unkey#2215
File: apps/dashboard/app/(app)/@breadcrumb/authorization/roles/[roleId]/page.tsx:28-29
Timestamp: 2024-11-10T16:45:16.994Z
Learning: In `authorization/roles/[roleId]/update-role.tsx`, the tag `role-${role.id}` is revalidated after updating a role to ensure that the caching mechanism is properly handled for roles.
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Go API Local / test_agent_local
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (59)
apps/dashboard/app/(app)/audit/page.tsx (1)
1-1: Navigation component simplification looks goodThe replacement of the previous Navbar component with the new Navigation component creates a cleaner interface with direct props. This change aligns perfectly with the PR objective of creating a generic navigation component that reduces code duplication while maintaining the same functionality.
Also applies to: 16-16
apps/dashboard/app/(app)/settings/vercel/page.tsx (2)
2-2: Import updated to use the new Navigation component.The component import has been correctly updated to use the new Navigation component from the dedicated navigation folder, which aligns with the PR objective of creating a generic navigation component.
54-54: Simplified navigation implementation.The Navigation component provides a cleaner interface with simple props (href, name, icon) compared to the previous Navbar implementation, reducing complexity while maintaining the same functionality.
apps/dashboard/app/(app)/identities/navigation.tsx (1)
1-16: Looks good - clean implementation of the identities navigation componentThe Navigation component follows the new pattern described in the PR objectives, creating a reusable navigation structure for the identities section. The component properly uses the Navbar from the shared component library with appropriate breadcrumb structure and icon.
apps/dashboard/app/(app)/audit/navigation.tsx (1)
1-14: Navigation component implementation looks goodThe Navigation component for the audit section is well-structured and consistent with the pattern being implemented across the application. The component correctly uses the Navbar with appropriate breadcrumb and icon.
apps/dashboard/app/(app)/ratelimits/navigation.tsx (1)
1-17: Good implementation with appropriate action buttonThe Navigation component for ratelimits follows the same pattern as the other navigation components while appropriately including the CreateNamespaceButton in the actions section. This is a good example of how the navigation pattern can be extended for specific needs.
apps/dashboard/app/(app)/logs/navigation.tsx (1)
1-14: Clean implementation of logs navigationThe Navigation component for the logs section is well-implemented and follows the consistent pattern established across the application. The component correctly uses the appropriate icon and breadcrumb structure.
apps/dashboard/app/(app)/settings/billing/plans/navigation.tsx (1)
1-18: Clean implementation of reusable navigation component!The implementation creates a reusable navigation component specifically for the Plans page, with a clear breadcrumb structure and proper active state. The comment on line 6 effectively communicates the purpose and reusability of this component.
apps/dashboard/app/(app)/ratelimits/page.tsx (2)
6-6: Good modularization of navigationThe import of the dedicated Navigation component aligns well with the PR's objective to make navigation loosely tied to pages.
33-33: Clean replacement of inline navbar with modular componentThe implementation correctly replaces what was likely an inline Navbar implementation with the new modular Navigation component, reducing code duplication.
apps/dashboard/app/(app)/apis/navigation.tsx (2)
7-10: Well-defined props interfaceThe NavigationProps type is clear and provides flexibility to the Navigation component to handle different UI states.
12-23: Solid implementation with conditional renderingThe Navigation component is well-structured with properly organized breadcrumbs and actions sections. The conditional rendering of the CreateApiButton based on APIs length and isNewApi state is a good practice that improves UX.
apps/dashboard/app/(app)/identities/page.tsx (2)
13-13: Good modularization of navigationThe import of the dedicated Navigation component aligns well with the PR's objective to make navigation loosely tied to pages.
41-41: Clean replacement of inline navbar with modular componentThe implementation correctly replaces what was likely an inline Navbar implementation with the new modular Navigation component, reducing code duplication.
apps/dashboard/app/(app)/apis/[apiId]/settings/navigation.tsx (1)
10-35: Verify prop type consistency with its usage.The component accepts an
apiprop of typeApi, which is correctly used throughout. However, make sure any dependent files also pass the completeapiobject, not just an ID or partial data.Additionally, the component properly provides contextual navigation with breadcrumbs and relevant actions, which aligns well with the PR's goal of standardizing navigation.
apps/dashboard/components/navigation/navigation.tsx (1)
6-17: Well-designed reusable navigation component.This component provides a simple, reusable navigation pattern for sections that need minimal breadcrumb structure. It aligns well with the PR objective to create generic navigation components and reduce duplicated code.
apps/dashboard/app/(app)/identities/[identityId]/navigation.tsx (1)
1-26: Well-structured navigation component for identity pages.The new
Navigationcomponent is clean and focused, handling navigation for the identity details page. It effectively implements breadcrumbs with proper truncation for long identity IDs (with thew-[200px] truncateclasses).apps/dashboard/app/(app)/apis/[apiId]/navigation.tsx (1)
10-32: Consider handling potential nullkeyAuthIdgracefully.The component looks well-structured and provides good functionality with breadcrumbs and actions. However, line 28 uses a non-null assertion (
!) forapi.keyAuthId, which could cause runtime errors if it's null.Consider adding a null check or providing a fallback:
-<CreateKeyButton apiId={api.id} keyAuthId={api.keyAuthId!} /> +{api.keyAuthId && <CreateKeyButton apiId={api.id} keyAuthId={api.keyAuthId} />}Or if a
CreateKeyButtonshould always be present, ensure thatkeyAuthIdis never null in the API type definition or provide proper error handling.apps/dashboard/app/(app)/authorization/permissions/[permissionId]/page.tsx (2)
8-8: Good import of dedicated Navigation component.The new import follows the established pattern of specific navigation components for unique cases.
66-66: Simplified Navigation implementation reduces complexity.Replacing what was likely a more complex navigation structure with this dedicated component improves readability and maintainability while reducing duplication.
apps/dashboard/app/(app)/logs/page.tsx (3)
4-4: Switched to using consistent icon library.Using the
@unkey/iconspackage maintains consistency across the application.
8-8: Using shared Navigation component follows PR objective.This import shows the dual approach mentioned in the PR: using the generic Navigation component from a shared location for simpler navigation scenarios.
23-23: Simplified navigation implementation with generic component.The generic
Navigationcomponent with basic props (href, name, icon) makes this implementation very clean and maintainable compared to what was likely a more complex structure before.apps/dashboard/app/(app)/authorization/permissions/navigation.tsx (1)
1-28: Well-structured, reusable navigation component.This new Navigation component is well designed with clear props and provides a consistent navigation experience for the permissions section. The comment on line 9 suggests this pattern will be reused elsewhere.
apps/dashboard/app/(app)/settings/root-keys/[keyId]/navigation.tsx (1)
1-22: Clean implementation with good attention to UI details.The navigation component correctly handles key ID display with appropriate truncation and styling. The breadcrumb structure provides clear context for the user's location within the settings.
apps/dashboard/app/(app)/apis/page.tsx (2)
7-7: Import looks good.The Navigation component import correctly replaces the previous Navbar import.
47-47:Details
✅ Verification successful
Verify that all previous Navbar functionality is preserved.
The Navigation component now receives
isNewApiandapisLengthprops. Ensure that the Navigation component implements all the functionality that was previously provided by the Navbar, including any actions like the CreateApiButton that was previously imported separately.
🏁 Script executed:
#!/bin/bash # Check if Navigation component includes CreateApiButton functionality rg -A 10 "export function Navigation" "./apps/dashboard/app/(app)/apis/navigation.tsx"Length of output: 486
Navigation component functionality is preserved; CreateApiButton is correctly integrated.
The update passes both
isNewApiandapisLengthto Navigation, and the component renders theCreateApiButtonwithin<Navbar.Actions>using a conditional (defaultOpen={apisLength === 0 || isNewApi}) that correctly mirrors the previous behavior.
- The
CreateApiButtonis present in the Navigation component.- The conditional logic for opening the CreateApiButton matches the intended functionality (triggered when there are no APIs or a new API is indicated).
apps/dashboard/app/(app)/settings/root-keys/new/page.tsx (2)
6-6: Import looks good.The Navigation component import correctly replaces the previous Navbar import.
32-32:Details
✅ Verification successful
Consider whether navigation context is needed.
The Navigation component is used without props here. Verify that this provides sufficient context to users about their location in the app (previously handled by Navbar breadcrumbs).
🏁 Script executed:
#!/bin/bash # Check the implementation of the root-keys Navigation component rg -A 10 "export function Navigation" "./apps/dashboard/app/(app)/settings/root-keys/navigation.tsx"Length of output: 506
Navigation Context Confirmed
The verification shows that the
<Navigation />component renders the complete navigation context—including breadcrumbs with an active "Settings" link and a primary action to create a new root key—ensuring users clearly understand their location in the app. No additional props are required at this time.
- File:
apps/dashboard/app/(app)/settings/root-keys/new/page.tsx(Line 32)- Component Details: The
Navigationcomponent internally renders<Navbar.Breadcrumbs>and<Navbar.Actions>, replicating the previous functionality of Navbar breadcrumbs.apps/dashboard/app/(app)/settings/billing/plans/page.tsx (1)
10-10: Navigation component implementation looks good!The change replaces the previous navigation structure with a dedicated Navigation component, aligning with the PR objective of creating a loosely coupled, reusable navigation system. This approach reduces code duplication and improves maintainability.
Also applies to: 23-23
apps/dashboard/app/(app)/settings/general/page.tsx (1)
3-3: Clean implementation of the new Navigation pattern!The change properly implements the new Navigation component with appropriate props (href, name, and icon), following the consistent pattern across the codebase. This approach creates a cleaner separation of concerns while maintaining the necessary context for the settings page.
Also applies to: 29-29
apps/dashboard/app/(app)/authorization/permissions/page.tsx (1)
14-14: Good implementation of context-aware Navigation component!The Navigation component is now provided with the
numberOfPermissionsas a prop, allowing it to maintain relevant page-specific information while adhering to the consistent navigation pattern. This approach effectively supports the PR's goal of reducing duplicated code while still preserving page-specific context.Also applies to: 59-59
apps/dashboard/app/(app)/identities/[identityId]/page.tsx (1)
23-23: Proper implementation of dynamic Navigation component!The change correctly passes the
identityIdto the Navigation component, which ensures that page-specific context is maintained while benefiting from the consistent navigation structure. This approach successfully balances the need for standardization with the requirements of contextual navigation.Also applies to: 60-60
apps/dashboard/app/(app)/settings/user/page.tsx (2)
2-2: Import change looks good.The import of the new
Navigationcomponent from the central location aligns with the PR objective of creating a reusable navigation component.
29-29: Navigation component implementation is clean.The
Navigationcomponent successfully replaces the previous navbar with a simpler interface that takes only the essential props:href,name, andicon. This aligns with the PR's goal of reducing code duplication and creating a loosely-coupled navigation system.apps/dashboard/app/(app)/settings/team/page.tsx (2)
24-24: Import change looks good.The import of the new
Navigationcomponent from the central location aligns with the PR objective of creating a reusable navigation component.
54-54: Navigation component implemented correctly in conditional rendering.The
Navigationcomponent is properly implemented in the conditional rendering path when no organization is present.apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/page.tsx (2)
23-23: Import of local navigation component looks good.Using a context-specific navigation implementation aligns with the PR's goal of maintaining individual navigation files in their respective folder structures for specific cases.
264-264: Specialized Navigation component implementation is appropriate.The specialized
Navigationcomponent takes context-specific objects (apiandapiKey) rather than simple props, which is appropriate for this more complex navigation context. This aligns with the PR's approach of using specialized navigation components where needed.apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/page.tsx (2)
6-6: Import of local navigation component looks good.Using a context-specific navigation implementation aligns with the PR's goal of maintaining individual navigation files in their respective folder structures for specific cases.
30-30: Specialized Navigation component implementation is appropriate.The specialized
Navigationcomponent takes context-specific parameters (apiIdandkeyA) rather than simple props, which is appropriate for this more complex navigation context. This aligns with the PR's approach of using specialized navigation components where needed.apps/dashboard/app/(app)/settings/root-keys/[keyId]/page.tsx (2)
11-11: Clean import of the new Navigation component.The import statement correctly references the Navigation component from a relative path.
130-130: Navigation component implementation looks good.The Navigation component is cleanly implemented, passing just the keyId prop which simplifies the interface compared to the previous detailed Navbar implementation with breadcrumbs.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/navigation.tsx (1)
1-53: Well-structured Navigation component with clear type definitions.The Navigation component implements a clean breadcrumb structure with properly typed props and clear navigation hierarchy. The component effectively:
- Uses the Navbar component for consistent UI
- Properly constructs the breadcrumb path from APIs to the current context
- Includes the API identifier in a badge with a copy button for convenience
This implementation aligns with the PR objective of creating a generic navigation component that can be reused across the application.
apps/dashboard/app/(app)/apis/[apiId]/page.tsx (2)
14-14: Clean import of the Navigation component.The import statement correctly references the Navigation component from a relative path.
140-140: Navigation component implementation looks good.The Navigation component is cleanly implemented, passing the api object which contains all necessary information for constructing the navigation elements. This simplifies the interface compared to the previous detailed Navbar implementation.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/page.tsx (2)
9-9: Clean import of the Navigation component.The import statement correctly references the Navigation component from a relative path.
36-36: Navigation component implementation looks good.The Navigation component is cleanly implemented, passing both the apiId and keyAuth object (as keyA) which contain all necessary information for constructing the navigation elements. This implementation is consistent with the refactoring pattern applied throughout the application.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/navigation.tsx (1)
17-20: The NavigationProps interface doesn't match its usage.The component's interface defines
apiandapiKeyprops, which aligns with how they're used in the component.apps/dashboard/app/(app)/settings/root-keys/page.tsx (1)
38-38:Details
❓ Verification inconclusive
Verify if Navigation component needs props.
The
Navigationcomponent is being used without props, which is different from its usage in other files. Ensure this is intentional and that the component has appropriate defaults.
🏁 Script executed:
#!/bin/bash # Search for the Navigation component definition in the root-keys folder fd "navigation.tsx" apps/dashboard/app/\\(app\\)/settings/root-keys --exec cat {} \;Length of output: 267
Verify Navigation component's props usage
It appears the script execution failed due to shell escaping issues with parentheses, so we couldn’t confirm the component definition automatically. Please manually verify that the
Navigationcomponent is not missing any required props. In particular, ensure that its implementation either sets appropriate default props or does not expect props—especially since it’s used differently in other parts of the codebase. You may re-run a corrected search (e.g., using quotes around the folder path) to inspect the component’s definition and confirm its intended usage.apps/dashboard/app/(app)/settings/billing/page.tsx (2)
44-44: LGTM: Navigation implementation is consistent with the PR objective.The component usage with
href,name, andiconprops aligns with the PR goal of creating a simplified, reusable navigation structure.
2-2:Details
❓ Verification inconclusive
Import path differs from other files.
This file imports Navigation from "@/components/navigation/navigation" while other files import from local "navigation" files. Verify if this is the intended pattern.
🏁 Script executed:
#!/bin/bash # Check how Navigation is imported across the codebase rg "import.*Navigation.*from" apps/dashboard # Check if the shared Navigation component exists fd "navigation.tsx" apps/dashboard/componentsLength of output: 3045
Clarification on Navigation Import Patterns
In the dashboard’s settings routes (e.g. billing, user, general, team, vercel), the Navigation component is imported using the absolute path (@/components/navigation/navigation), which is consistent across these files. In contrast, other areas (such as several API and nested routes like billing/plans or root-keys) import Navigation using a relative path (e.g../navigationor../navigation). Please confirm that this differentiation is intentional. If the shared Navigation component is meant for top-level settings while a locally scoped version is used in other contexts, consider documenting this design decision to avoid potential confusion.apps/dashboard/app/(app)/apis/[apiId]/settings/page.tsx (1)
14-14: Implementation follows PR objective for navigation simplification.The change replaces the previous navigation approach with a more specific
Navigationcomponent that is properly passed the API object. This aligns with the PR objective to create loosely coupled navigation components for specific cases.Also applies to: 57-57
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/page.tsx (1)
11-11: Navigation component implementation is consistent with PR objective.The new approach correctly passes both the
apiIdandapiKeyobjects to theNavigationcomponent, maintaining the necessary context while simplifying the navigation structure.Also applies to: 44-44
apps/dashboard/app/(app)/authorization/permissions/[permissionId]/navigation.tsx (1)
1-69: Well-structured reusable navigation component with appropriate UI features.This navigation component implementation includes:
- Clear breadcrumb hierarchy for authorization context
- Appropriate tooltip handling for long permission names
- Copy functionality for permission IDs and names
- Integration with delete functionality
The comment on line 9 ("Reusable for settings where we only change the link") accurately describes the purpose of this component.
apps/dashboard/app/(app)/authorization/roles/[roleId]/page.tsx (1)
5-5: Navigation implementation follows consistent pattern.The change properly implements the same navigation pattern seen across other files, passing the role object to the component. This contributes to the overall goal of reducing code duplication as mentioned in the PR objectives.
Based on the retrieved learning, remember that updating a role involves revalidating the tag
role-${role.id}for proper cache handling. Ensure that the new Navigation component preserves this behavior if it contains role update functionality.Also applies to: 109-109
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/navigation.tsx (1)
1-59: Well-structured navigation component with clear organization.The Navigation component is designed with clear typing, logical breadcrumb structure, and appropriate actions. This implementation aligns well with the PR objective of creating a generic navigation component that can be reused across different pages.
The breadcrumb structure provides a clear path for users to navigate through the application hierarchy, and the actions section offers useful functionality with the copy button and create key button.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/navigation.tsx (1)
47-47: Handle potential null keyAuthId value safely.You're using a non-null assertion operator (!) on
keyA.api.keyAuthId, but the type definition on line 14 indicates this property can be null. This could lead to runtime errors if a null value is encountered.Consider handling the null case explicitly:
-<CreateKeyButton apiId={keyA.api.id} keyAuthId={keyA.api.keyAuthId!} /> +{keyA.api.keyAuthId ? ( + <CreateKeyButton apiId={keyA.api.id} keyAuthId={keyA.api.keyAuthId} /> +) : ( + // Fallback UI or disabled state when keyAuthId is null + <Button disabled>Create Key</Button> +)}apps/dashboard/app/(app)/authorization/roles/page.tsx (1)
13-13: Successful implementation of the navigation refactoring.The addition of the Navigation component and its integration into the page structure successfully implements the PR objective of making navigation loosely tied to pages. This change simplifies the UI by moving the complex navigation logic into a dedicated component.
Also applies to: 59-59
| <Navbar.Breadcrumbs.Link href={`/authorization/permissions/${role.id}`} isIdentifier active> | ||
| {role.id} | ||
| </Navbar.Breadcrumbs.Link> |
There was a problem hiding this comment.
Fix incorrect link in breadcrumb navigation.
The link to permissions is using the role ID, but it should likely be linking to a role-specific page instead:
- <Navbar.Breadcrumbs.Link href={`/authorization/permissions/${role.id}`} isIdentifier active>
+ <Navbar.Breadcrumbs.Link href={`/authorization/roles/${role.id}`} isIdentifier active>
{role.id}
</Navbar.Breadcrumbs.Link>This appears to be a copy-paste error as you're in the roles section but linking to the permissions route.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Navbar.Breadcrumbs.Link href={`/authorization/permissions/${role.id}`} isIdentifier active> | |
| {role.id} | |
| </Navbar.Breadcrumbs.Link> | |
| <Navbar.Breadcrumbs.Link href={`/authorization/roles/${role.id}`} isIdentifier active> | |
| {role.id} | |
| </Navbar.Breadcrumbs.Link> |
chronark
left a comment
chronark
left a comment
There was a problem hiding this comment.
This is nice, I haven't had time to check the deployment yet, but I like the refactor
|
@chronark lemme check the pages one by one then i'll merge |
ogzhanolguncu
left a comment
ogzhanolguncu
left a comment
There was a problem hiding this comment.
Pages look good I checked on preview.
3 participants
What does this PR do?
I created a generic navigation.tsx in our components for the simple navigation that is reused multiple times. Any non generic got their own
navigation.tsxin their respective folder structure to pass props around.It looks like a lot of code but we actually remove code that was reused, in mutiple places like "settings".
Fixes # (issue)
Fixes Main issues at the same time.
Type of change
How should this be tested?
Click the dashboard like crazy! It worked for me, I tried all of screens to make sure none of them were broken.
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Refactor