Conversation
ogzhanolguncu
requested review from
MichaelUnkey,
chronark,
mcstepp and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis pull request removes legacy sidebar components ( Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (9)
🪧 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: 2
🧹 Nitpick comments (12)
apps/dashboard/hooks/use-mobile.tsx (3)
5-19: Consider refining the mobile detection implementationThe hook correctly detects mobile viewport width, but there's an inconsistency in how it's implemented. The event listener uses
matchMediabut the actual check useswindow.innerWidth.For consistency, consider using the
matchesproperty from the MediaQueryList:export function useIsMobile() { const [isMobile, setIsMobile] = React.useState<boolean | undefined>(undefined); React.useEffect(() => { const mql = window.matchMedia(`(max-width: ${MOBILE_BREAKPOINT - 1}px)`); const onChange = () => { - setIsMobile(window.innerWidth < MOBILE_BREAKPOINT); + setIsMobile(mql.matches); }; mql.addEventListener("change", onChange); - setIsMobile(window.innerWidth < MOBILE_BREAKPOINT); + setIsMobile(mql.matches); return () => mql.removeEventListener("change", onChange); }, []); return !!isMobile; }This approach is more consistent and reduces the chance of edge case bugs where the two methods might return different results.
3-3: Consider using Tailwind's breakpoint for consistencyThe constant
MOBILE_BREAKPOINTis set to 768px, which aligns with medium (md) breakpoint in many CSS frameworks. If you're using Tailwind CSS in this project, consider using their breakpoint values for consistency across the codebase:-const MOBILE_BREAKPOINT = 768; +// Match Tailwind's md breakpoint +const MOBILE_BREAKPOINT = 768;This ensures that your JavaScript breakpoints match your CSS breakpoints.
6-6: Initial state could be determined synchronously for SSR compatibilitySetting the initial state to
undefinedworks but could cause a brief flash of UI during rendering. Since this hook is for client-side detection, you might want to consider improving SSR compatibility:-const [isMobile, setIsMobile] = React.useState<boolean | undefined>(undefined); +const [isMobile, setIsMobile] = React.useState<boolean>(false);Then in your useEffect, you could add a check to prevent the hook from running during SSR:
React.useEffect(() => { if (typeof window === 'undefined') return; // rest of the code }, []);This prevents hydration mismatches and improves the predictability of your UI during server-side rendering.
apps/dashboard/components/navigation/navbar.tsx (1)
80-99: Conditional button for icon or sidebar toggle.
Switching between a customiconand the sidebar toggle button is clever. Consider adding labels or tooltips for improved accessibility.+ <Button + aria-label="Toggle sidebar" + onClick={toggleSidebar} + variant="outline" + className="size-6 p-0 [&>svg]:size-[18px] bg-gray-4 hover:bg-gray-5" + > + {isCollapsed ? ( + <SidebarLeftShow size="md-medium" /> + ) : ( + <SidebarLeftHide size="md-medium" /> + )} + </Button>apps/dashboard/components/app-sidebar.tsx (3)
42-50: Consider clarifying or removing the placeholder logic increateNestedNavigation.
Currently, this function just returnsbaseNavwithout processing. If the intention is to nest items in the future, it might be clearer to rename the function or add a comment indicating upcoming logic.
193-249: Validate screen-reader compatibility forAnimatedLoadingSpinner.
Since this spinner changes its opacity dynamically, ensure it won't cause confusion for screen-reader users. Consider addingrole="status"andaria-live="polite"or visually hiding text that informs the user of loading.
266-291: Check SSR compatibility with runtime style injection.
Appending<style>todocument.headworks only in a browser environment (typeof document !== "undefined"). For server-rendered pages, consider adding the animation styling to a global CSS file or a server-safe approach to avoid hydration mismatch.apps/dashboard/components/navigation/sidebar/workspace-navigations.tsx (2)
62-62: Confirm correctness of switching from a different icon toNodes.
Previously, the summary mentioned removing theNodesicon. However, the code now usesNodes. Verify this aligns with your new design choice; otherwise, remove references.
126-126: Mark external links more clearly.
UsingBookBookmarkfor docs is good, but consider an external-link icon or label. This helps emphasize navigation outside your app.apps/dashboard/components/ui/sidebar.tsx (3)
19-34: Handle cookie fallback or server-side rendering.
Storing sidebar state in a cookie is convenient, but confirm SSR usage. If the server tries to read the cookie for an initial collapsed state, you may need to ensure synchronization between server and client for a smooth user experience.
47-149: Encapsulate or validate thedefaultOpenread from cookies.
Right now, the state is inlined with a default open prop. If you plan to read from cookies initially, consider a function that checks for a valid Boolean to prevent possible parse errors from malformed cookies.
90-105: Review the keyboard shortcut event logic.
Currently,Ctrl/Cmd + btoggles the sidebar. Ensure no collisions with browser/OS shortcuts. Consider making this binding configurable to respect user preferences or possible conflicts.
📜 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 (20)
apps/dashboard/app/(app)/apis/navigation.tsx(1 hunks)apps/dashboard/app/(app)/desktop-sidebar.tsx(0 hunks)apps/dashboard/app/(app)/layout.tsx(2 hunks)apps/dashboard/app/(app)/mobile-sidebar.tsx(0 hunks)apps/dashboard/components.json(1 hunks)apps/dashboard/components/app-sidebar.tsx(1 hunks)apps/dashboard/components/navigation/navbar.tsx(3 hunks)apps/dashboard/components/navigation/sidebar/sidebar-mobile.tsx(1 hunks)apps/dashboard/components/navigation/sidebar/team-switcher.tsx(7 hunks)apps/dashboard/components/navigation/sidebar/user-button.tsx(2 hunks)apps/dashboard/components/navigation/sidebar/workspace-navigations.tsx(5 hunks)apps/dashboard/components/ui/sidebar.tsx(1 hunks)apps/dashboard/hooks/use-mobile.tsx(1 hunks)apps/dashboard/package.json(1 hunks)internal/icons/src/icons/circle-question.tsx(1 hunks)internal/icons/src/icons/sidebar-left-hide.tsx(1 hunks)internal/icons/src/icons/sidebar-left-show.tsx(1 hunks)internal/icons/src/index.ts(1 hunks)internal/icons/src/props.ts(1 hunks)internal/ui/tailwind.config.js(1 hunks)
💤 Files with no reviewable changes (2)
- apps/dashboard/app/(app)/desktop-sidebar.tsx
- apps/dashboard/app/(app)/mobile-sidebar.tsx
🧰 Additional context used
🧬 Code Definitions (9)
apps/dashboard/app/(app)/apis/navigation.tsx (1)
apps/dashboard/components/navigation/navbar.tsx (1) (1)
Navbar(46-59)
internal/icons/src/icons/sidebar-left-hide.tsx (1)
internal/icons/src/props.ts (2) (2)
IconProps(26-30)sizeMap(7-24)
internal/icons/src/icons/sidebar-left-show.tsx (1)
internal/icons/src/props.ts (2) (2)
IconProps(26-30)sizeMap(7-24)
apps/dashboard/app/(app)/layout.tsx (4)
apps/dashboard/components/ui/sidebar.tsx (1) (1)
SidebarProvider(591-591)apps/dashboard/components/app-sidebar.tsx (1) (1)
AppSidebar(153-191)apps/dashboard/components/navigation/sidebar/sidebar-mobile.tsx (1) (1)
SidebarMobile(9-29)internal/ui/src/components/empty.tsx (1) (1)
Empty(6-18)
apps/dashboard/components/ui/sidebar.tsx (1)
apps/dashboard/hooks/use-mobile.tsx (1) (1)
useIsMobile(5-19)
apps/dashboard/components/navigation/sidebar/sidebar-mobile.tsx (4)
apps/dashboard/components/ui/sidebar.tsx (1) (1)
useSidebar(593-593)internal/icons/src/icons/sidebar-left-show.tsx (1) (1)
SidebarLeftShow(15-63)apps/dashboard/components/navigation/sidebar/team-switcher.tsx (1) (1)
WorkspaceSwitcher(31-173)apps/dashboard/components/navigation/sidebar/user-button.tsx (1) (1)
UserButton(20-111)
apps/dashboard/components/navigation/sidebar/team-switcher.tsx (1)
apps/dashboard/components/ui/sidebar.tsx (1) (1)
useSidebar(593-593)
apps/dashboard/components/navigation/navbar.tsx (2)
apps/dashboard/components/ui/sidebar.tsx (1) (1)
useSidebar(593-593)internal/ui/src/components/button.tsx (1) (1)
Button(443-443)
internal/icons/src/icons/circle-question.tsx (1)
internal/icons/src/props.ts (2) (2)
IconProps(26-30)sizeMap(7-24)
🔇 Additional comments (33)
internal/ui/tailwind.config.js (1)
79-79: Added "base" color to the generateRadixColors functionThe addition of the "base" color to the color names array enhances the available color palette for the application. This new color will be available throughout the application via Tailwind classes.
apps/dashboard/package.json (1)
100-100: Updated tailwind-merge dependency to version 2.5.4Good update to the tailwind-merge package. This ensures the application uses a more recent version which may include bug fixes, performance improvements, and potentially new features for handling Tailwind class merging.
apps/dashboard/app/(app)/apis/navigation.tsx (1)
13-13: Simplified Navbar.Breadcrumbs by removing the icon propThe icon prop was removed from the Navbar.Breadcrumbs component, which aligns with the PR's objective of updating the navigation logic. This simplification makes the code cleaner while maintaining functionality.
internal/icons/src/index.ts (1)
53-55: Added new icon exports for sidebar functionalityThe addition of three new icon exports (
circle-question,sidebar-left-show, andsidebar-left-hide) supports the new sidebar component functionality being implemented in this PR. These icons will likely be used for toggling sidebar visibility and help functionality.internal/icons/src/icons/sidebar-left-hide.tsx (1)
1-63: New icon component is well-structuredThis component is well-implemented following the established pattern for icon components in the project. It properly:
- Utilizes the IconProps interface
- Leverages the sizeMap for consistent sizing
- Has appropriate SVG attributes and viewBox
- Uses proper SVG structure with clear semantic elements
The visual design with the vertical line, polyline, and rectangle effectively represents a "hide sidebar" action.
internal/icons/src/icons/sidebar-left-show.tsx (1)
1-63: New icon component properly implements show sidebar functionalityThis component is correctly implemented, matching the pattern of other icon components. The key difference from the SidebarLeftHide icon is in the polyline points (line 40), which create an arrow pointing in the opposite direction to visually indicate "show sidebar" functionality.
The component is properly structured with appropriate SVG attributes and follows the project's icon implementation conventions.
internal/icons/src/icons/circle-question.tsx (1)
1-53: Question mark icon implementation follows project conventionsThe CircleQuestion icon is well-implemented, following the same pattern as other icon components in the codebase. It correctly:
- Uses the IconProps interface with appropriate defaults
- Leverages the sizeMap for consistent sizing
- Has appropriate SVG attributes
- Creates a visually distinct question mark in a circle using proper SVG elements
The implementation is clean and follows the project's established conventions for icon components.
apps/dashboard/components.json (3)
5-5: Added TypeScript JSX support.The addition of
"tsx": trueproperly enables TypeScript JSX support for the dashboard components, which is a good practice for type safety.
10-11: Tailwind CSS configuration update.The addition of CSS variables and empty prefix settings provides better flexibility for styling components and prevents potential class name conflicts.
15-18: New import aliases added.These additional aliases (
ui,lib, andhooks) provide convenient shortcuts for importing components, utilities, and hooks. This makes the codebase more maintainable by reducing import path complexity.apps/dashboard/components/navigation/sidebar/sidebar-mobile.tsx (3)
9-14: Clean conditional rendering for mobile view.The component correctly uses the
isMobilestate from the sidebar context to conditionally render, ensuring this component only appears on mobile devices.
17-26: Mobile sidebar implementation with appropriate UI elements.The component provides essential navigation elements for mobile: a sidebar toggle button, workspace switcher, and user menu. The button for opening the sidebar uses appropriate styling and icon.
24-26: Future implementation comment.There's a TODO comment indicating plans for additional functionality. Consider creating a task to track this for the next iteration.
Would you like me to help create a task for implementing the mentioned indicator in the next iteration?
apps/dashboard/app/(app)/layout.tsx (5)
1-3: New sidebar component imports.Replacing legacy sidebar components with new implementations (
AppSidebar,SidebarMobile, andSidebarProvider) aligns with the PR's goal of updating the navigation system.
32-32: Updated base layout styling.Changed background from
bg-backgroundtobg-base-12to maintain consistent styling with the new sidebar components.
34-38: Desktop sidebar implementation.The layout now properly uses the
SidebarProviderto manage sidebar state and renders the newAppSidebarcomponent with appropriate styling through className.
39-47: Main content and mobile sidebar structure.The content area is well-organized with clear comments and proper positioning of the mobile sidebar at the top of content when on mobile devices.
48-67: Preserved workspace status handling.The conditional rendering based on workspace enabled status has been properly maintained, ensuring a consistent user experience when workspaces are disabled.
internal/icons/src/props.ts (2)
3-3: Added "medium" weight option.The addition of a "medium" weight option to the
Weighttype enhances the flexibility of icon styling, allowing for more nuanced visual hierarchies.
9-9: New medium weight icon size configurations.Consistently added medium weight entries with 1.5 stroke width across all size variants. This provides a balanced option between the thin (1.0) and regular (2.0) weights.
Also applies to: 13-13, 17-17, 21-21
apps/dashboard/components/navigation/sidebar/team-switcher.tsx (3)
3-3: Imports look consistent.
These imports are utilized seamlessly in the subsequent code. No duplication or conflicts observed.Also applies to: 13-13, 14-14, 16-16, 17-17, 18-18, 20-20
41-41: Correct usage of the sidebar state.
UsingisMobileandstatefromuseSidebarto defineisCollapsedis clear and maintains separate behaviors for desktop vs. mobile.Also applies to: 43-44
74-86: Effective styling and tooltip condition logic.
Good job applying conditional classes for theDropdownMenuTriggerand rendering tooltips/icons based on the sidebar's collapsed state. This approach keeps the UI responsive and user-friendly.Also applies to: 101-102, 115-115, 119-119
apps/dashboard/components/navigation/sidebar/user-button.tsx (5)
12-13: New imports for sidebar logic.
Bringing inuseSidebarandcnaligns with the revised architecture and seems consistent.
32-33: Concise user display name logic.
Combiningusername,fullName, andprimaryEmailAddressunder a single variable is a clean design choice.
37-43: Conditional rendering for desktop vs. mobile.
Dynamically adjusting layout (avatar, username, chevron) based onisCollapsedand mobile state delivers a unified user experience. Good use of fallback avatar handling.Also applies to: 45-65, 68-69
72-76: Dropdown alignment.
Declaringside="bottom"and setting responsive widths (max-w-xs,md:w-96) effectively handles both mobile and desktop needs.
23-23:Details
✅ Verification successful
Sidebar collapsed state handling.
The condition(state === "collapsed" || isMobile) && !(isMobile && openMobile)appears correct for setting the collapsed UI on mobile vs. desktop.Run the following script to confirm
openMobileusage is consistent across other components:Also applies to: 25-26
🏁 Script executed:
#!/bin/bash # Searching references to openMobile in the codebase rg -A 4 'openMobile'Length of output: 2965
Verified
openMobileusage consistency across components.The usage of
openMobilein the condition(state === "collapsed" || isMobile) && !(isMobile && openMobile)inapps/dashboard/components/navigation/sidebar/user-button.tsxaligns with its usage inapps/dashboard/components/ui/sidebar.tsx. No changes are needed here.apps/dashboard/components/navigation/navbar.tsx (2)
3-3: Importing icons anduseSidebar.
The new imports forSidebarLeftHide,SidebarLeftShow, anduseSidebaralign with the expanded/collapsed sidebar logic.Also applies to: 8-8
33-33: Optional breadcrumb icon and sidebar toggling logic.
Allowing an optionaliconis flexible, and usinguseSidebarfor controlling the breadcrumb-based sidebar toggle is neatly integrated.Also applies to: 69-70, 73-74
apps/dashboard/components/navigation/sidebar/workspace-navigations.tsx (2)
74-74: Review alignment of replaced icons with intended meanings.
Icons likeShieldKey,InputSearch,Grid,Layers3,Sparkle3, andGeareach symbolize different concepts. Make sure these new icons match the menu labels and user expectations.Also applies to: 81-81, 87-87, 94-94, 101-101, 116-116
16-19:Details
✅ Verification successful
Ensure icon type changes don’t break existing references.
icon: React.ElementType;is broader than the removedLucideIcontype. Confirm no existing code relies on strict Lucide types, and verify external references compile properly.
🏁 Script executed:
#!/bin/bash # Search codebase for usage patterns of NavItem.icon to confirm that # references match the updated type. rg -A 3 'NavItem'Length of output: 4507
Icon type change verified—no refactoring required.
After searching the codebase, the updated
icon: React.ElementType;inapps/dashboard/components/navigation/sidebar/workspace-navigations.tsxappears to be used consistently across the project (e.g., inapp-sidebar.tsxanddashboard/navbar.tsx). There’s no evidence that any existing references rely on the stricterLucideIcontype, and nothing in the search output indicates compilation issues related to this change.apps/dashboard/components/ui/sidebar.tsx (1)
575-595: Overall, the modular structure of the new Sidebar components is excellent.
It’s cohesive and follows best practices with a context-driven approach. Code is well-organized, ensuring good maintainability and consistent UI patterns across the app.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/dashboard/components/app-sidebar.tsx (4)
33-41: Consider consolidating conditional class handling ingetButtonStyles.
While this utility function effectively applies different Tailwind classes based onisActiveorshowLoader, you might streamline readability by extracting shared or repeated classes and grouping conditional logic. This can help new contributors quickly parse the style rules.
53-151: Split or modularizeNavItemsfor clarity.
TheNavItemscomponent handles both single-level and nested items. While the logic is correct, consider extracting the collapsible logic (lines 91–151) into a dedicated sub-component for easier maintenance and testing.
223-295: Loading spinner logic is visually appealing but consider timing and performance.
UsingsetIntervalat 125ms is acceptable for a short rotor, yet consider whether using pure CSS animations might simplify code. Alternatively, a more React-friendly approach (likeuseReducerorrequestAnimationFrame) might further reduce re-renders.
297-321: Runtime CSS injection works but review potential duplication.
Handling custom animations via appended<style>ensures the spinner's motion preference respect. However, extracting these rules into a shared CSS or style module might reduce overhead if multiple components adopt a similar pattern.apps/dashboard/components/ui/sidebar.tsx (1)
47-149: Persisting the 'open' state in cookies is helpful; consider reading cookie on mount.
This approach sets the cookie but does not initialize from an existing cookie. If you want to preserve the user’s previous session state, read this cookie on mount to sync the default. Otherwise, your defaults here may override user preferences.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/dashboard/components/app-sidebar.tsx(1 hunks)apps/dashboard/components/navigation/sidebar/team-switcher.tsx(7 hunks)apps/dashboard/components/navigation/sidebar/user-button.tsx(2 hunks)apps/dashboard/components/ui/sidebar.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/dashboard/components/navigation/sidebar/team-switcher.tsx
- apps/dashboard/components/navigation/sidebar/user-button.tsx
🧰 Additional context used
🧬 Code Definitions (2)
apps/dashboard/components/app-sidebar.tsx (4)
apps/dashboard/components/navigation/sidebar/workspace-navigations.tsx (3) (3)
NavItem(16-26)createWorkspaceNavigation(56-122)resourcesNavigation(124-137)apps/dashboard/components/ui/sidebar.tsx (10) (10)
SidebarMenuButton(587-587)SidebarMenuSub(589-589)SidebarMenuSubItem(591-591)SidebarMenuSubButton(590-590)Sidebar(577-577)SidebarHeader(584-584)SidebarContent(578-578)SidebarGroup(580-580)SidebarMenu(585-585)SidebarFooter(579-579)internal/icons/src/icons/sidebar-left-show.tsx (1) (1)
SidebarLeftShow(15-63)internal/icons/src/icons/sidebar-left-hide.tsx (1) (1)
SidebarLeftHide(15-63)
apps/dashboard/components/ui/sidebar.tsx (1)
apps/dashboard/hooks/use-mobile.tsx (1) (1)
useIsMobile(5-19)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (12)
apps/dashboard/components/app-sidebar.tsx (3)
1-32: Imports look consistent and purposeful.
All imports appear to be in use, and the structure is clean. This ensures a clear separation of responsibilities for each module and sets a solid foundation for the component logic that follows.
43-51: Placeholder function for nested navigation is acceptable.
createNestedNavigationsimply returnsbaseNavhere, suggesting future expansions might add sub-items. The straightforward approach is fine for maintainability.
153-221:AppSidebaris well-structured; verify defaults for different screen sizes.
This component nicely integrates core sidebar elements. The toggle button logic for collapsed vs. expanded states is clear. Just ensure that the default collapsed state, or any custom user preferences, is respected in all scenarios (mobile vs. desktop).apps/dashboard/components/ui/sidebar.tsx (9)
1-18: Imports and initial setup are coherent.
The file correctly brings in Radix UI, class-variance-authority, and supporting hooks. This scaffolds the sidebar’s advanced behavior.
19-24: Cookie constants are clearly defined.
Retention of sidebar state in a cookie with a one-weekmax-ageis straightforward. Ensure the lifetime aligns with broader product requirements (e.g. user preferences or compliance rules).
26-36: Context type is well-addressed.
SidebarContextPropssuccinctly captures needed fields (state,open,openMobile, etc.). No issues noted.
38-45: Custom hook usage constraint is appropriate.
Throwing an error ifuseSidebaris used outsideSidebarProviderensures no invalid usage. This is a best practice to prevent unexpected null context references.
151-253:Sidebarcomponent transitions cleanly between desktop and mobile.
Excellent approach handling collapsed vs. mobile states. The usage ofSheetfor mobile is consistent with Radix UI standards, ensuring a strong responsive pattern.
258-299: Header, Footer, and Separator sub-components are elegantly minimal.
These small wrappers neatly enforce spacing and style. Extended customization for each region remains flexible.
301-385: Structural components (Content, Group, Labels, Actions) are straightforward.
The separate sub-components (SidebarContent,SidebarGroup, etc.) effectively isolate responsibilities and avoid large monolithic code blocks.
388-432: Menu and menu-item structures, plus CVA usage, are well implemented.
ThesidebarMenuButtonVariantsclearly delineates styling across states and sizes. This cva approach is helpful for consistent design tokens throughout the app.
434-595: Menu actions, sub-menus, and exports are comprehensive.
Your tooltip logic inSidebarMenuButtonis well-handled, particularly with conditional string vs. object usage. Sub-components (SidebarMenuAction,SidebarMenuSub,SidebarMenuSubItem, andSidebarMenuSubButton) are neatly separated, facilitating easy updates. The final exports confirm a clear public API for the sidebar module.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
apps/dashboard/components/navigation/sidebar/sidebar-mobile.tsx (1)
18-19: Add an accessible label to the sidebar toggle button.Providing an
aria-labelon this button can help screen readers identify its purpose.<Button variant="ghost" onClick={() => setOpenMobile(true)} + aria-label="Open sidebar" className="[&_svg]:size-[20px]" >apps/dashboard/components/app-sidebar.tsx (2)
33-103: Segment theAppSidebarlogic further to reduce complexity.Even though the structure is modular, consider extracting some logic (e.g., header content or resource navigation) into sub-components. This improves maintainability and testability, especially as the sidebar grows.
-export function AppSidebar({...props}: ... & { workspace: Workspace }) { - // Entire logic +export function AppSidebar({...props}: ... & { workspace: Workspace }) { + const headerContent = <AppSidebarHeader workspace={props.workspace} />; + // ... + return ( + <Sidebar ...> + {headerContent} + // ... + </Sidebar> + ); +} +function AppSidebarHeader({ workspace }: { workspace: Workspace }) { + // ... +}
219-247: Optimize sub-route navigation if frequently accessed.If the user navigates heavily between routes, consider prefetching links (e.g., Next.js
Linkprefetch) to speed up transitions. This is especially helpful if the sub-routes are used often.apps/dashboard/components/ui/sidebar.tsx (3)
19-25: Consider using local or session storage instead of cookies for UI state
Storing UI preferences in cookies sends them on every request. If this data doesn’t need to be sent to the server, local or session storage may be simpler.
90-92: Watch for conflicts with common “Ctrl + B” shortcut
“Ctrl + B” is often used to toggle bold text in many editors and applications, which could surprise users. Consider making the keyboard shortcut configurable or using a different combination.
525-539: Naming clarification for “SidebarMenuSub”
While not strictly incorrect, “SidebarMenuSub” can be ambiguous. Consider something more specific if nested submenus become common.
📜 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 (5)
apps/dashboard/components/app-sidebar.tsx(1 hunks)apps/dashboard/components/navigation/sidebar/sidebar-mobile.tsx(1 hunks)apps/dashboard/components/navigation/sidebar/team-switcher.tsx(7 hunks)apps/dashboard/components/navigation/sidebar/user-button.tsx(2 hunks)apps/dashboard/components/ui/sidebar.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/components/navigation/sidebar/team-switcher.tsx
🧰 Additional context used
🧬 Code Definitions (2)
apps/dashboard/components/navigation/sidebar/sidebar-mobile.tsx (4)
apps/dashboard/components/ui/sidebar.tsx (1) (1)
useSidebar(594-594)internal/icons/src/icons/sidebar-left-show.tsx (1) (1)
SidebarLeftShow(15-63)apps/dashboard/components/navigation/sidebar/team-switcher.tsx (1) (1)
WorkspaceSwitcher(31-176)apps/dashboard/components/navigation/sidebar/user-button.tsx (1) (1)
UserButton(19-107)
apps/dashboard/components/ui/sidebar.tsx (1)
apps/dashboard/hooks/use-mobile.tsx (1) (1)
useIsMobile(5-19)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Build / Build
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
apps/dashboard/components/navigation/sidebar/sidebar-mobile.tsx (1)
9-14: Ensure this component is tested thoroughly.Currently, there's no explicit test coverage for the mobile-only behavior. Consider adding tests (e.g., using React Testing Library) to verify that this component returns
nullin non-mobile contexts and renders the UI in mobile contexts.You can create a companion test file and confirm the rendering of this component under mobile vs. non-mobile breakpoints or configurations.
apps/dashboard/components/navigation/sidebar/user-button.tsx (2)
35-67: Consider adding a small test for the dropdown trigger in collapsed vs. open states.Tests can cover:
- Ensuring the trigger is center-aligned in collapsed mode.
- Verifying the username and chevron appear correctly in expanded mode.
22-25:Details
❓ Verification inconclusive
Validate
isMobilestate transitions.The logic that combines
isMobilewithopenMobileis clear, but ensure dynamic resizing scenarios are handled (e.g., if a user resizes from desktop to mobile mid-session). This might require an additional effect to synchronize.Please verify if the
useSidebarhook recalculatesisMobileon viewport resize, and that user experience remains consistent.
🏁 Script executed:
#!/bin/bash # Searching for the isMobile usage and checking if there's any event listener for viewport resizing rg 'isMobile'Length of output: 2634
Ensure dynamic resizing is handled for isMobile state
After reviewing the repository, we confirmed that the
isMobilevalue is derived from the customuseMobile(and indirectly viauseSidebar) hook. However, there is no clear indication—via an event listener or media query listener—that this hook actively recalculates its value on viewport resizing. This raises concerns about whether dynamic changes (e.g., switching from desktop to mobile mid-session) are properly handled.
- in
apps/dashboard/hooks/use-mobile.tsx: Verify that the hook incorporates logic (e.g., an event listener for the window resize event or a media query listener) to updateisMobilewhen the viewport changes.- in
apps/dashboard/components/navigation/sidebar/user-button.tsx: Confirm that theisCollapsedcomputation reflects these potential state transitions promptly.Please double-check the implementation of
useSidebarand its underlying hooks. If dynamic viewport changes aren’t currently managed, consider adding the necessary effect to updateisMobileaccordingly.apps/dashboard/components/app-sidebar.tsx (2)
355-379: Kudos for supporting motion preferences.This block respects the
prefers-reduced-motionsetting by halting animations—this is an excellent accessibility practice.
249-322:Details
❓ Verification inconclusive
Verify nested item state resets on route changes.
The logic that sets
subPending[subItem.label] = trueand then resets it may not catch all route transition scenarios (e.g., if the user navigates away quickly or the route fails). Ensure this does not leave any pending load states.Here's a script that can search for other references to
subPendingto see if there's a fallback or error handling:
🏁 Script executed:
#!/bin/bash ast-grep --pattern $'$_ = $_.subPending[$_] = $_'Length of output: 50
Action: Confirm Robust State Reset on Route Transition Failures
- The current implementation resets the loading state for nested items using a fixed 300ms delay after invoking
router.pushwithin astartTransitioncallback.- There is no additional error handling or fallback mechanism if the route navigation fails or the user navigates away quickly, which may leave a pending state active.
- Please verify manually that under all route transition scenarios (including failures and quick navigations), the loading state for nested items is reliably reset. Consider adding error handling or leveraging router events (e.g., cancellation or error callbacks) to ensure state consistency.
apps/dashboard/components/ui/sidebar.tsx (5)
70-88: Good job allowing external control and internal state
The approach of using both an internal_openstate and optional externalopen&onOpenChangeprops is a flexible pattern that accommodates multiple use cases.
95-105: Clean keyboard event handling
Attaching and removing thekeydownlistener on mount/unmount is well implemented. It reduces the risk of memory leaks and ensures the shortcut is properly cleaned up.
187-208: Mobile sheet implementation looks good
Switching to aSheeton mobile devices is a neat solution. The layout remains consistent, and responsiveness is well managed throughisMobileandsetOpenMobile.
211-253: Refactor suggestion reiterated
This multi-layered approach (group,peer, and data attributes) is powerful but can be complex for future maintainers. Consider simplifying or documenting the layering strategy if your team finds it hard to maintain.
576-595: Exports are cleanly organized
Exporting all sidebar-related components, types, and hooks in one place promotes discoverability and modular usage throughout the codebase.
|
@perkinsjr time to break 😄 |
|
Looks fine, To be clear this is V1 of the sidebar without the nested options like the design? |
|
Yeah we do it in multiple passes to make it easier to review |
|
Well then this is fine. |
|
I think this is in the wrong thread LMAO |
|
opps |
|
Nested routes are a bit problematic we have to do data fetching etc... PR would be too big. I plan to go route by route like, apis nested routes first, then ratelimits etc... |
|
Sorry about that boys 🫡 |
|
@MichaelUnkey :pepeout: |
|
ignore me |
|
😄 |
* feat: add new sidebar * fix: icon * fix: padding issues * feat: add new sidebar * fix: ui issues * chore: clenaup * chore: remove unused button * fix: collapse issue * fix: ui issues * chore: remove unused files
* feat: add usage-insight on the desktop-sidebar * remove console.log * refactor: improve code consistency and formatting across multiple files * fix: keep only the progress circle, remove the particle effect, keep the card always open * refactor: simplify layout and desktop sidebar components, remove unused billing logic * feat(billing): add usage fetching hook and TRPC query for billing usage data * fix: update workspace creation logic to set plan to 'free' and improve error handling * fix: remove unnecessary console log for usage query fetching * refactor: adjust padding and height in usage insights component for improved layout * fix: update plan type in usage insights to handle workspace tier and default to 'Free' * [autofix.ci] apply automated fixes * refactor: improve code readability and formatting in usage insights component * refactor: remove Particles component and relocate useMousePosition hook * refactor: enhance code formatting and readability in desktop sidebar component * fix: remove use-mouse hook * pnpm fmt * pnpm-lock fix * refactor: update usage insights to use 'tier' instead of 'plan' and simplify loading state handling * fix: remove outdated and wrong quota check (#3000) we now have proper quotas, so this check is redundand. If a workspace is disabled, there is already a warning in the root layout to contact us. * Update README.md (#3002) Fix Invalid URL of "Read our blog post" * feat: sidebar (#3003) * feat: add new sidebar * fix: icon * fix: padding issues * feat: add new sidebar * fix: ui issues * chore: clenaup * chore: remove unused button * fix: collapse issue * fix: ui issues * chore: remove unused files * fix: layout shifts and missing props (#3006) * fix: layout shift * fix: missing active tags * refactor: clean up imports and improve layout component structure * pnpm fmt * refactor: simplify usage query options in useFetchUsage hook * pnpm fmt * fix: broken lock * fix: quotas rename to quota This fixes the incorrect usage max issue * fix: refetch usage --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Oğuzhan Olguncu <21091016+ogzhanolguncu@users.noreply.github.com> Co-authored-by: James P <jamesperkins@hey.com> Co-authored-by: Andreas Thomas <dev@chronark.com> Co-authored-by: okuma <49895482+kumachan99@users.noreply.github.com> Co-authored-by: ogzhanolguncu <ogzhan11@gmail.com>
4 participants
What does this PR do?
Fixes # (issue)
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
Summary by CodeRabbit
New Features
UI Enhancements
Refactor
Bug Fixes