init sidebar refactor #218
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request involves the removal of several components related to sidebar functionality in a web application, including Changes
Possibly related PRs
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Outside diff range and nitpick comments (39)
packages/ui/src/hooks/use-mobile.tsx (3)
1-4: Consider centralizing breakpoint configurationThe
MOBILE_BREAKPOINTconstant should ideally be sourced from a centralized theme configuration to maintain consistency across the application.Consider:
- Moving this to a shared theme configuration
- Using modern React imports:
import { useState, useEffect } from 'react'
5-9: Optimize initial state handlingThe current implementation initializes
isMobileasundefined, which could lead to an unnecessary re-render when the actual value is set.Consider initializing the state with the current window width:
-const [isMobile, setIsMobile] = React.useState<boolean | undefined>( - undefined, -); +const [isMobile, setIsMobile] = React.useState<boolean>( + () => typeof window !== 'undefined' && window.innerWidth < MOBILE_BREAKPOINT +);
20-21: Simplify boolean coercionThe double negation operator (
!!) is used to coerce the value to boolean, but this could be simplified if we initialize the state properly as suggested above.If you implement the earlier suggestion to initialize state as boolean, you can simply:
-return !!isMobile; +return isMobile;apps/webservice/src/app/[workspaceSlug]/AppSidebarPopover.tsx (2)
9-23: Consider accessibility and keyboard navigation improvements.While the basic popover implementation is good, there are several accessibility enhancements that could be made:
- Add ARIA attributes for better screen reader support
- Implement keyboard navigation handlers
- Consider focus management when the popover opens/closes
Consider applying these improvements:
export const AppSidebarPopover: React.FC = () => { const { activeSidebarItem } = useSidebarPopover(); return ( - <Popover open={activeSidebarItem != null}> + <Popover + open={activeSidebarItem != null} + modal={true} + aria-label="Sidebar content" + > <PopoverAnchor className="w-full" /> <PopoverContent side="right" sideOffset={1} - className="h-[100vh] w-[300px] border-y-0 border-l-0 bg-black" + className="h-[100vh] w-[300px] border-y-0 border-l-0 bg-black focus:outline-none" + onOpenAutoFocus={(e) => e.preventDefault()} + onCloseAutoFocus={(e) => e.preventDefault()} > {activeSidebarItem} </PopoverContent> </Popover> ); };
14-18: Consider making dimensions configurable via props.The hardcoded dimensions (height: 100vh, width: 300px) might not be flexible enough for different viewport sizes or use cases.
Consider making these configurable:
export const AppSidebarPopover: React.FC = () => { + interface Props { + width?: string; + className?: string; + } const { activeSidebarItem } = useSidebarPopover(); return ( <Popover open={activeSidebarItem != null}> <PopoverAnchor className="w-full" /> <PopoverContent side="right" sideOffset={1} - className="h-[100vh] w-[300px] border-y-0 border-l-0 bg-black" + className={cn( + "h-[100vh] border-y-0 border-l-0 bg-black", + width ?? "w-[300px]", + className + )} > {activeSidebarItem} </PopoverContent> </Popover> ); };packages/ui/unused.css (2)
2-23: Consider standardizing color value formats.The color values use inconsistent formats:
- Some use percentages (e.g.,
0 0% 98%)- Some don't have percentages (e.g.,
240 5.9 10)This could lead to maintenance issues. Consider standardizing all color values to use the same format.
:root { --sidebar-background: 0 0% 98%; --sidebar-foreground: 240 5.3% 26.1%; - --sidebar-primary: 240 5.9% 10%; + --sidebar-primary: 240 5.9% 10.0%; --sidebar-primary-foreground: 0 0% 98%; --sidebar-accent: 240 4.8% 95.9%; - --sidebar-accent-foreground: 240 5.9% 10%; + --sidebar-accent-foreground: 240 5.9% 10.0%; --sidebar-border: 220 13% 91%; - --sidebar-ring: 217.2 91.2% 59.8% + --sidebar-ring: 217.2 91.2% 59.8%; }
24-31: Consider merging the two @layer base declarations.There are two separate
@layer basedeclarations that could be combined for better maintainability.@layer base { :root { /* ... root variables ... */ } .dark { /* ... dark theme variables ... */ } -} -@layer base { * { @apply border-border; } body { @apply bg-background text-foreground; } }packages/ui/tailwind.config.ts (1)
14-24: LGTM! Consider adding color token documentationThe color token structure is well-organized and follows consistent naming patterns. The separation of primary, accent, and utility colors provides good flexibility for the sidebar styling.
Consider adding a comment block above the sidebar colors to document:
- The purpose of each color token
- Expected usage patterns
- Any specific accessibility considerations
colors: { + // Sidebar color tokens + // - DEFAULT: Main background color + // - foreground: Default text color + // - primary: Used for active/selected states + // - accent: Used for hover/interactive states + // - utility: border and ring colors for layout and focus states sidebar: {apps/webservice/src/app/[workspaceSlug]/SettingsSidebar.tsx (3)
9-11: Add prop validation for workspace data.Consider adding validation to handle cases where workspace data might be undefined or incomplete, especially since the component relies on
workspace.slug.export const SettingsSidebar: React.FC<{ workspace: Workspace }> = ({ workspace, }) => { + if (!workspace?.slug) { + return null; // or a fallback UI + }
13-26: Consider improving navigation and accessibility.
- The back navigation link could benefit from additional accessibility attributes.
- The hover state removes background but might need focus states for keyboard navigation.
<Link href={`/${workspace.slug}`} - className="flex w-full items-center gap-2 text-left hover:bg-transparent" + className="flex w-full items-center gap-2 text-left hover:bg-transparent focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring" + aria-label={`Back to ${workspace.slug} workspace`} >
1-47: Add component documentation and tests.As this is a new component in a sidebar refactor:
- Add JSDoc documentation describing the component's purpose and usage
- Consider adding integration tests for navigation flows
Would you like me to help generate:
- Component documentation template?
- Integration test suite setup?
apps/webservice/src/app/[workspaceSlug]/_SidebarPopoverSystem.tsx (2)
8-11: Add prop validation and consider type isolationConsider these improvements for better type safety and maintainability:
- Add runtime prop validation using
zodorprop-types- Consider creating a dedicated interface for the workspace prop instead of using the database schema type directly
Example implementation:
import { z } from 'zod'; interface WorkspaceProps { slug: string; // Add only the necessary workspace properties used by this component } const propsSchema = z.object({ systemId: z.string().min(1), workspace: z.object({ slug: z.string().min(1), }), }); export const SidebarPopoverSystem: React.FC<z.infer<typeof propsSchema>> = ({ workspace, systemId }) => { // Validate props at runtime propsSchema.parse({ workspace, systemId }); // ... rest of the component };
1-51: Consider performance and accessibility improvementsTo enhance the component's overall quality:
- Implement virtualization for large lists using
react-virtualor similar- Add ARIA labels and roles for better accessibility
- Consider memoizing the component to prevent unnecessary re-renders
- Add keyboard navigation support
Example implementation for keyboard navigation:
const SidebarPopoverSystem: React.FC<Props> = memo(({ workspace, systemId }) => { const handleKeyDown = (e: React.KeyboardEvent) => { switch(e.key) { case 'ArrowDown': // Focus next item break; case 'ArrowUp': // Focus previous item break; } }; return ( <div role="navigation" aria-label="System navigation" onKeyDown={handleKeyDown} > {/* ... existing content ... */} </div> ); });apps/webservice/src/app/[workspaceSlug]/AppSidebarPopoverContext.tsx (3)
16-19: Consider using a more descriptive no-op functionThe default context implementation is good, but the no-op function could be more explicit about being a default value.
const AppSidebarPopoverContext = createContext<AppSidebarPopoverContextType>({ activeSidebarItem: null, - setActiveSidebarItem: () => {}, + setActiveSidebarItem: () => { + if (process.env.NODE_ENV !== 'production') { + console.warn('AppSidebarPopoverContext: setActiveSidebarItem was called without a Provider'); + } + }, });
21-21: Add error handling to the hookThe hook should throw an error when used outside of the provider context.
-export const useSidebarPopover = () => useContext(AppSidebarPopoverContext); +export const useSidebarPopover = () => { + const context = useContext(AppSidebarPopoverContext); + if (context === undefined) { + throw new Error('useSidebarPopover must be used within an AppSidebarPopoverProvider'); + } + return context; +};
38-47: Consider adding mouse enter handler for consistencyThe
SidebarWithPopovercomponent handlesonMouseLeavebut notonMouseEnter. This might lead to inconsistent behavior if the sidebar is entered without hovering over a specific item.export const SidebarWithPopover: React.FC<{ children: React.ReactNode; }> = ({ children }) => { const { setActiveSidebarItem } = useSidebarPopover(); return ( <Sidebar + onMouseEnter={() => setActiveSidebarItem(null)} onMouseLeave={() => setActiveSidebarItem(null)} > {children} </Sidebar> ); };apps/webservice/src/app/globals.css (1)
71-78: Consider using a softer black for dark mode background.Pure black (
0 0% 0%) can create excessive contrast and cause eye strain. Consider using a softer shade like240 10% 3.9%(matching the existing dark theme background) for better readability and user comfort.- --sidebar-background: 0 0% 0%; + --sidebar-background: 240 10% 3.9%;apps/webservice/src/app/[workspaceSlug]/SidebarNavMain.tsx (3)
23-39: Improve type safety and validationConsider the following improvements to the component's interface:
- Replace
anywith a proper icon component type- Add URL validation
- Consider making required fields explicit
+import { TablerIconsProps } from '@tabler/icons-react'; + +type IconComponent = React.ComponentType<TablerIconsProps>; + export const SidebarNavMain: React.FC<{ title: string; items: { popoverId?: string; title: string; url: string; - icon?: any; + icon?: IconComponent; isOpen?: boolean; items?: { popoverId?: string; - icon?: any; + icon?: IconComponent; title: string; url: string; exact?: boolean; }[]; }[]; }> = ({ title, items }) => {
40-88: Consider adding error boundaries and loading statesThe component could benefit from error handling and loading states to improve user experience.
Consider wrapping the component with an error boundary and adding loading states:
- Create an ErrorBoundary component
- Add loading states for dynamic content
- Handle cases where items prop might be undefined or empty
Would you like me to provide an example implementation?
49-49: Extract hardcoded CSS classes to constants or CSS modulesThe hardcoded CSS classes
text-neutral-400should be extracted to maintain consistency and ease of maintenance.+const ICON_CLASS_NAME = "text-neutral-400"; + // In the component -{item.icon && <item.icon className="text-neutral-400" />} +{item.icon && <item.icon className={ICON_CLASS_NAME} />} -{subItem.icon && <subItem.icon className="text-neutral-400" />} +{subItem.icon && <subItem.icon className={ICON_CLASS_NAME} />}Also applies to: 71-71
apps/webservice/src/app/[workspaceSlug]/AppSidebar.tsx (3)
97-97: Fix typo in "Deployements"The word "Deployements" is misspelled and should be "Deployments".
- title: "Deployements", + title: "Deployments",
86-112: Consider optimizing system navigation structureThe current implementation has several areas for improvement:
- Placeholder URLs should be replaced with actual routes
- Consider memoizing the navigation structure to prevent unnecessary recalculations
- The mapping could be moved outside the component to improve readability
Consider refactoring to:
const getSystemNavigation = (systems: System[], baseUrl: string) => systems.map((s) => ({ title: s.name, url: `${baseUrl}/systems/${s.id}`, items: [ { title: "Environments", icon: IconPlant, url: `${baseUrl}/systems/${s.id}/environments`, }, // ... other items ], })); // In component: const navSystem = useMemo( () => getSystemNavigation(systems.items, `/${workspace.slug}`), [systems.items, workspace.slug] );
80-84: Consider implementing data prefetchingMultiple API calls in the component could impact initial render performance. Consider implementing data prefetching at the page level or using React Suspense with proper loading states.
You could:
- Move data fetching to a page-level loader
- Implement proper loading states
- Use React Suspense boundaries
apps/webservice/src/app/[workspaceSlug]/AppSidebarCreateMenu.tsx (1)
Line range hint
94-94: Add handler for Execute Runbook menu item.The
Execute Runbookmenu item appears to be incomplete as it lacks an associated dialog or handler.Would you like me to help implement the Execute Runbook functionality or create an issue to track this task?
apps/webservice/src/app/[workspaceSlug]/AppSidebarHeader.tsx (3)
Line range hint
3-26: Consider organizing imports for better maintainabilityConsider grouping imports into these categories for better maintainability:
- External dependencies
- Internal UI components
- Local components/utilities
"use client"; + // External dependencies import type { System, Workspace } from "@ctrlplane/db/schema"; import Link from "next/link"; import { IconCheck, IconChevronDown, IconSearch } from "@tabler/icons-react"; import { signOut } from "next-auth/react"; + // Internal UI components import { Button } from "@ctrlplane/ui/button"; import { DropdownMenu, DropdownMenuContent, // ... other dropdown imports } from "@ctrlplane/ui/dropdown-menu"; import { SidebarMenu, SidebarMenuItem } from "@ctrlplane/ui/sidebar"; + // Local components/utilities import { api } from "~/trpc/react"; import { SearchDialog } from "./_components/SearchDialog"; import { AppSidebarCreateMenu } from "./AppSidebarCreateMenu";
Line range hint
61-67: Add loading and error handling for workspace switchingThe workspace switching functionality should handle loading and error states to provide better user feedback.
{workspaces.map((ws) => ( <Link key={ws.id} href={`/${ws.slug}`} passHref - onClick={() => update.mutate({ activeWorkspaceId: ws.id })} + onClick={() => { + update.mutate( + { activeWorkspaceId: ws.id }, + { + onError: (error) => { + // Add error toast notification + console.error('Failed to switch workspace:', error); + } + } + ); + }} + className={update.isLoading ? 'opacity-50 cursor-wait' : ''} >
108-112: Add aria-label to search buttonImprove accessibility by adding an aria-label to the search button.
<SearchDialog> <Button variant="ghost" size="icon" + aria-label="Open search dialog" className="h-6 w-6 flex-shrink-0" > <IconSearch className="h-4 w-4" /> </Button> </SearchDialog>apps/webservice/src/app/[workspaceSlug]/_SidebarSystems.tsx (4)
31-34: Improve localStorage implementationConsider these enhancements for better maintainability and type safety:
- Use a namespaced key prefix for localStorage
- Use actual boolean values instead of string literals
+const STORAGE_KEY_PREFIX = 'ctrlplane.sidebar.systems'; const [open, setOpen] = useLocalStorage( - `sidebar-systems-${system.id}`, - "false", + `${STORAGE_KEY_PREFIX}.${system.id}`, + false, );
38-39: Simplify boolean comparisonThe string comparison with "true" can be simplified by using actual boolean values.
-open={open === "true"} -onOpenChange={() => setOpen(open === "true" ? "false" : "true")} +open={Boolean(open)} +onOpenChange={(newState) => setOpen(newState)}
81-83: Consider persisting the systems list collapsed stateFor consistency with SystemCollapsible, consider persisting the open/closed state of the systems list.
-const [open, setOpen] = useState(true); +const [open, setOpen] = useLocalStorage( + `${STORAGE_KEY_PREFIX}.systemsList`, + true +);
84-89: Enhance accessibility with ARIA labelsAdd appropriate ARIA labels to improve accessibility for screen readers.
<CollapsibleTrigger + aria-label={`${open ? 'Collapse' : 'Expand'} systems list`} className="flex items-center gap-1 text-xs text-muted-foreground"> Your systems <IconChevronRight + aria-hidden="true" className={cn("h-3 w-3", open && "rotate-90", "transition-all")} /> </CollapsibleTrigger>packages/ui/src/sheet.tsx (2)
1-18: LGTM! Consider grouping imports by type.The imports and basic component definitions are well-structured. The "use client" directive is correctly placed for Next.js client components.
Consider grouping imports by type for better organization:
"use client"; - import type { VariantProps } from "class-variance-authority"; - import * as React from "react"; - import * as SheetPrimitive from "@radix-ui/react-dialog"; - import { Cross2Icon } from "@radix-ui/react-icons"; - import { cva } from "class-variance-authority"; + // React and types + import * as React from "react"; + import type { VariantProps } from "class-variance-authority"; + + // Third-party libraries + import * as SheetPrimitive from "@radix-ui/react-dialog"; + import { Cross2Icon } from "@radix-ui/react-icons"; + import { cva } from "class-variance-authority";
57-76: Consider extracting close button styles to a separate constant.The SheetContent implementation is solid, but the close button styles could be more maintainable.
Consider extracting the close button styles:
+const closeButtonStyles = "absolute right-4 top-4 rounded-sm opacity-70 ring-offset-background transition-opacity hover:opacity-100 focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-2 disabled:pointer-events-none data-[state=open]:bg-secondary"; + const SheetContent = React.forwardRef< React.ElementRef<typeof SheetPrimitive.Content>, SheetContentProps >(({ side = "right", className, children, ...props }, ref) => ( <SheetPortal> <SheetOverlay /> <SheetPrimitive.Content ref={ref} className={cn(sheetVariants({ side }), className)} {...props} > - <SheetPrimitive.Close className="absolute right-4 top-4 rounded-sm opacity-70 ring-offset-background transition-opacity hover:opacity-100 focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-2 disabled:pointer-events-none data-[state=open]:bg-secondary"> + <SheetPrimitive.Close className={closeButtonStyles}>packages/ui/src/sidebar.tsx (6)
88-89: Consider using localStorage instead of cookies for client-side stateStoring the sidebar state in a cookie may not be necessary since it's a client-side preference. Using
localStoragecan simplify the code and avoid unnecessary cookie usage, which is also sent with every HTTP request.Apply this refactor:
- document.cookie = `${SIDEBAR_COOKIE_NAME}=${openState}; path=/; max-age=${SIDEBAR_COOKIE_MAX_AGE}`; + localStorage.setItem(SIDEBAR_COOKIE_NAME, openState.toString());Also, update the initialization logic to read from
localStorageinstead of cookies:- const [_open, _setOpen] = React.useState(defaultOpen); + const [_open, _setOpen] = React.useState(() => { + const storedState = localStorage.getItem(SIDEBAR_COOKIE_NAME); + return storedState !== null ? storedState === 'true' : defaultOpen; + });
104-107: Reconsider the keyboard shortcut to avoid conflictsThe keyboard shortcut
Ctrl/Cmd + Bis commonly used for toggling bold text in many applications and may conflict with browser or application defaults. Consider choosing an alternative shortcut to prevent user confusion or unintended behavior.
274-293: Enhance accessibility with ARIA attributes inSidebarTriggerAdding appropriate ARIA attributes such as
aria-labelandaria-expandedcan improve accessibility for users relying on screen readers.Apply this diff to include ARIA attributes:
const { toggleSidebar, open } = useSidebar(); <Button ref={ref} data-sidebar="trigger" variant="ghost" size="icon" className={cn("h-7 w-7", className)} + aria-label="Toggle Sidebar" + aria-expanded={open} onClick={(event) => { onClick?.(event); toggleSidebar(); }} {...props} >
751-761: Consolidate export statements for better readabilityFor improved maintainability and readability, consider grouping related exports together or exporting all components at once.
Apply this refactor:
export { Sidebar, SidebarContent, SidebarFooter, SidebarGroup, SidebarGroupAction, SidebarGroupContent, SidebarGroupLabel, SidebarHeader, SidebarInput, SidebarInset, SidebarMenu, SidebarMenuAction, SidebarMenuBadge, SidebarMenuButton, SidebarMenuItem, SidebarMenuSkeleton, SidebarMenuSub, SidebarMenuSubButton, SidebarMenuSubItem, SidebarProvider, SidebarRail, SidebarSeparator, SidebarTrigger, useSidebar, };Alternatively, if you anticipate adding more components, you might consider exporting them as a single object or default export.
425-483: Improve semantic structure by using appropriate HTML elementsIn
SidebarGroupcomponents, using semantic HTML elements likenavorsectionwith appropriate ARIA roles can enhance accessibility and semantic meaning.For example, modify the
SidebarGroupcomponent:- <div + <nav ref={ref} data-sidebar="group" className={cn("relative flex w-full min-w-0 flex-col p-2", className)} {...props} - /> + />Ensure that any styling and behavior remain consistent after the change.
211-215: Type assertion forstyleprop may not be necessaryThe explicit type assertion to
React.CSSPropertiesmight be redundant since thestyleprop accepts this type by default. Removing the type assertion can simplify the code.Apply this diff:
style={ { "--sidebar-width": SIDEBAR_WIDTH_MOBILE, - } as React.CSSProperties + } }If TypeScript raises an error due to custom CSS properties, consider defining them in a custom type instead.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (27)
apps/webservice/src/app/[workspaceSlug]/AppSidebar.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/AppSidebarCreateMenu.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/AppSidebarHeader.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/AppSidebarPopover.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/AppSidebarPopoverContext.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/SettingsSidebar.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/SidebarContext.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/SidebarLink.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/SidebarMain.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/SidebarNavMain.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/SidebarPanels.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/SidebarPopoverSystem.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/SidebarPopoverTargets.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/SidebarSettings.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/SidebarSystems.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/SidebarWorkspace.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/_SidebarPopoverSystem.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/_SidebarSystems.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/layout.tsx(2 hunks)apps/webservice/src/app/globals.css(3 hunks)apps/webservice/tailwind.config.ts(1 hunks)packages/ui/package.json(3 hunks)packages/ui/src/hooks/use-mobile.tsx(1 hunks)packages/ui/src/sheet.tsx(1 hunks)packages/ui/src/sidebar.tsx(1 hunks)packages/ui/tailwind.config.ts(1 hunks)packages/ui/unused.css(1 hunks)
💤 Files with no reviewable changes (9)
- apps/webservice/src/app/[workspaceSlug]/SidebarContext.tsx
- apps/webservice/src/app/[workspaceSlug]/SidebarLink.tsx
- apps/webservice/src/app/[workspaceSlug]/SidebarMain.tsx
- apps/webservice/src/app/[workspaceSlug]/SidebarPanels.tsx
- apps/webservice/src/app/[workspaceSlug]/SidebarPopoverSystem.tsx
- apps/webservice/src/app/[workspaceSlug]/SidebarPopoverTargets.tsx
- apps/webservice/src/app/[workspaceSlug]/SidebarSettings.tsx
- apps/webservice/src/app/[workspaceSlug]/SidebarSystems.tsx
- apps/webservice/src/app/[workspaceSlug]/SidebarWorkspace.tsx
🔇 Additional comments (31)
packages/ui/src/hooks/use-mobile.tsx (1)
5-21: Verify hook usage in sidebar components
Let's ensure this hook is properly integrated with the new sidebar components mentioned in the PR summary.
✅ Verification successful
Hook is properly integrated with sidebar components
The useIsMobile hook is correctly integrated within the sidebar implementation:
- It's properly imported and used in the
SidebarProvidercomponent - The hook's value controls responsive behavior:
- Renders a sheet component on mobile devices
- Handles different sidebar widths for mobile/desktop
- Controls touch-friendly interactions on mobile
- The implementation follows best practices with proper cleanup of event listeners
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for useIsMobile hook usage in sidebar components
echo "Searching for useIsMobile usage in sidebar components..."
rg "useIsMobile" --type tsx -l | grep -i "sidebar"
# Verify proper responsive handling in sidebar components
echo "Checking responsive handling in sidebar components..."
rg "max-width.*768px|min-width.*768px" --type tsx -l | grep -i "sidebar"
Length of output: 417
Script:
#!/bin/bash
# Search for useIsMobile hook usage across all files
echo "Searching for useIsMobile hook usage..."
rg "useIsMobile"
# Search for sidebar-related files and their responsive handling
echo "Checking sidebar components..."
fd "sidebar" --type f -e ts -e tsx -e js -e jsx
# Check responsive handling patterns in these files
echo "Checking responsive patterns in sidebar files..."
fd "sidebar" --type f -e ts -e tsx -e js -e jsx --exec rg -l "matchMedia|max-width|min-width|MOBILE|isMobile" {}
Length of output: 1382
Script:
#!/bin/bash
# Let's examine the sidebar.tsx implementation to understand how useIsMobile is used
echo "Checking sidebar.tsx implementation..."
cat packages/ui/src/sidebar.tsx
# Let's also check if there are any other mobile-related imports or patterns in the app sidebars
echo "Checking app sidebars for mobile patterns..."
rg -A 3 "import.*mobile|isMobile|MOBILE|matchMedia" apps/webservice/src/app/[workspaceSlug]/AppSidebar.tsx apps/webservice/src/app/[workspaceSlug]/SettingsSidebar.tsx
Length of output: 23992
apps/webservice/src/app/[workspaceSlug]/AppSidebarPopover.tsx (2)
1-8: LGTM: Clean imports and proper client directive.
The imports are well-organized and the "use client" directive is correctly placed at the top of the file.
10-10: Verify error handling for missing context.
The component assumes the context will always be available. We should verify how this integrates with the parent components.
packages/ui/unused.css (1)
13-22: Consider differentiating ring colors between themes.
The --sidebar-ring color is identical in both light and dark themes (217.2 91.2% 59.8%). Consider if this is intentional or if the dark theme should have a different ring color for better contrast.
Let's check if this is consistent with other theme variables:
#!/bin/bash
# Description: Check if other ring colors differ between themes
# Search for ring color definitions in theme files
rg --type-add 'styles:*.{css,scss}' -t styles '(--.*ring.*:.*);'packages/ui/tailwind.config.ts (1)
16-23: Verify CSS custom properties are defined
The sidebar colors reference CSS custom properties (e.g., --sidebar-background, --sidebar-foreground) that must be defined elsewhere in the codebase.
Let's verify these CSS variables are properly defined:
✅ Verification successful
CSS custom properties for sidebar colors are properly defined
The CSS custom properties for the sidebar colors are correctly defined in the codebase. They are defined in the :root and .dark selectors within the base layer, as found in the CSS output:
:root {
--sidebar-background: 0 0% 98%;
--sidebar-foreground: 240 5.3% 26.1%;
--sidebar-primary: 240 5.9% 10%;
--sidebar-primary-foreground: 0 0% 98%;
--sidebar-accent: 240 4.8% 95.9%;
--sidebar-accent-foreground: 240 5.9% 10%;
--sidebar-border: 220 13% 91%;
--sidebar-ring: 217.2 91.2% 59.8%;
}
.dark {
--sidebar-background: 0 0% 0%;
--sidebar-foreground: 240 4.8% 95.9%;
--sidebar-primary: 224.3 76.3% 48%;
--sidebar-primary-foreground: 0 0% 100%;
--sidebar-accent: 240 3.7% 15.9%;
--sidebar-accent-foreground: 240 4.8% 95.9%;
--sidebar-border: 240 3.7% 15.9%;
--sidebar-ring: 217.2 91.2% 59.8%;
}All the CSS variables referenced in the Tailwind configuration are properly defined with appropriate values for both light and dark themes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CSS custom property definitions
rg --type css --type scss "(?:^|\s):root\s*{[^}]*--sidebar-(?:background|foreground|primary|accent|border|ring)" -A 10
Length of output: 304
Script:
#!/bin/bash
# Search for CSS custom property definitions with a simpler pattern
rg --type css --type scss "--sidebar-" -B 2 -A 2
# Also check for potential CSS module files
fd -e css -e scss . packages/ui
Length of output: 518
Script:
#!/bin/bash
# Search for CSS custom property definitions with correct quoting
rg -l "sidebar" packages/ui
# Check contents of any CSS-like files in the repository
fd -e css -e scss -e styles . | xargs cat
# Look for potential theme or style configurations
rg "createTheme|theme" -A 5 -B 5
Length of output: 33433
apps/webservice/tailwind.config.ts (2)
Line range hint 7-7: Verify UI package path resolution
The path to the UI package needs to be verified to ensure it correctly resolves during build time.
#!/bin/bash
# Description: Verify that the UI package path exists and contains relevant files
# Check if the UI package directory exists
if [ -d "../../packages/ui/src" ]; then
echo "UI package directory exists"
# List TypeScript/React files to verify content
find "../../packages/ui/src" -type f -name "*.ts" -o -name "*.tsx"
else
echo "Warning: UI package directory not found"
fi17-28: Consider documenting CSS custom properties
The sidebar color system looks well-structured with a comprehensive set of HSL variables. However, we should ensure these CSS custom properties are properly documented.
Let's verify the CSS custom properties are defined:
Consider adding a CSS documentation block in your styles that explains:
- The purpose of each sidebar color variable
- Expected HSL value ranges
- Usage examples in different sidebar states
This will help maintain consistency as the sidebar evolves.
✅ Verification successful
Let me try another search strategy to locate the CSS custom properties:
This broader search should help us locate where these custom properties might be defined, even if they're not in traditional CSS/SCSS files or if they're in a different directory structure than expected.
CSS custom properties are properly defined and documented
The sidebar CSS custom properties are well-defined in apps/webservice/src/app/globals.css with both light and dark theme variations. Each property has clear HSL values that follow a consistent pattern:
- Light theme values provide a clean, subtle color scheme (e.g.,
--sidebar-background: 0 0% 98%) - Dark theme values offer appropriate contrast (e.g.,
--sidebar-background: 0 0% 0%)
The properties are organized alongside other theme variables in a structured manner within the :root and .dark selectors, making the system maintainable and scalable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for CSS custom property definitions
# Search for sidebar CSS custom property definitions
rg --type css --type scss "(?:^|\s+)--sidebar-(?:background|foreground|primary|primary-foreground|accent|accent-foreground|border|ring)\s*:"
# Also check for any potential duplicate definitions
rg --type css --type scss "(?:^|\s+)--sidebar-" | sort | uniq -d
Length of output: 123
Script:
#!/bin/bash
# Cast a wider net to find any CSS/SCSS files first
fd -e css -e scss
# Then search all files (not just CSS/SCSS) for sidebar custom properties
# since they could be in other file types like tsx/jsx
rg "--sidebar-" -l
# Get content of any global style files that might contain these definitions
fd "global" -e css -e scss -x cat {}
Length of output: 4991
apps/webservice/src/app/[workspaceSlug]/SettingsSidebar.tsx (1)
1-7: LGTM! Well-organized imports following design system patterns.
The imports are properly structured, utilizing the internal design system components and types.
apps/webservice/src/app/[workspaceSlug]/layout.tsx (7)
5-5: LGTM: New sidebar component imports
The imports align with the sidebar refactoring objectives.
24-27: LGTM: Well-structured type definition
The Props type definition enhances type safety and code clarity.
29-32: LGTM: Improved parameter handling
The updated signature with direct destructuring of workspaceSlug improves code readability while maintaining type safety.
36-36: LGTM: Proper error handling in workspace fetching
The workspace fetching logic includes appropriate error handling with the notFound() fallback.
43-43: Verify AppSidebar required props
Let's check if the workspace prop is properly typed as required in the AppSidebar component.
✅ Verification successful
Based on the search results, I can see that AppSidebar component is properly typed with the required workspace prop of type Workspace. This is evident from the component definition in apps/webservice/src/app/[workspaceSlug]/AppSidebar.tsx:
export const AppSidebar: React.FC<{ workspace: Workspace }> = async ({
workspace,
}) => {The workspace prop is correctly typed as required in the AppSidebar component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check AppSidebar component props definition
ast-grep --pattern 'interface $_ {
workspace: $_
}'
Length of output: 281
Script:
#!/bin/bash
# Search for AppSidebar component definition and its props
rg -g "*.tsx" -g "*.ts" "AppSidebar" -A 5 -B 5
Length of output: 15427
16-17: Verify the existence of imported components
Let's ensure the new sidebar components exist in the correct locations.
✅ Verification successful
Imports verified successfully
Both AppSidebar.tsx and AppSidebarPopoverContext.tsx exist in the correct location at apps/webservice/src/app/[workspaceSlug]/ directory, and they are properly referenced throughout the codebase with consistent usage patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of new sidebar components
fd -e tsx -e ts "AppSidebar|AppSidebarPopoverContext" apps/webservice/src/app/
Length of output: 281
Script:
#!/bin/bash
# Search for AppSidebar file
fd "AppSidebar.tsx" apps/webservice/src/app/
# Search for AppSidebarPopoverContext file
fd "AppSidebarPopoverContext.tsx" apps/webservice/src/app/
# Alternative search using ripgrep to find any references
rg -g "*.{tsx,ts}" "AppSidebar|AppSidebarPopoverContext" apps/webservice/src/app/
Length of output: 2953
40-57: Verify the provider nesting order
The component introduces a new nested provider structure. Let's verify this is the correct order of providers and that no provider dependencies are violated.
✅ Verification successful
Provider nesting order is correct
The nesting order is appropriate based on the context usage patterns found:
AppSidebarPopoverProvider(outermost) is only used for popover state managementTerminalSessionsProvider(middle) is used by terminal-related components and target drawerSidebarProvider(innermost) provides the layout structure for the sidebar components
The current nesting doesn't show any dependency violations, as components access their respective contexts without cross-provider dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for provider dependencies and usage
# Look for useContext hooks in the components to verify proper nesting
rg -t tsx -t ts "useContext.*(?:AppSidebarPopover|TerminalSessions|Sidebar)Context" apps/webservice/src/
Length of output: 281
Script:
#!/bin/bash
# Search for context usage in TypeScript/React files
rg -t typescript -t javascriptreact -t typescriptreact "useContext.*(?:AppSidebarPopover|TerminalSessions|Sidebar)Context" apps/webservice/src/
# Search for context definitions
rg -t typescript -t javascriptreact -t typescriptreact "createContext.*(?:AppSidebarPopover|TerminalSessions|Sidebar)" apps/webservice/src/
# Search for provider component definitions
ast-grep --pattern 'const $_Provider = ({ children }: { children: ReactNode }) => {
$$$
}'
Length of output: 452
Script:
#!/bin/bash
# Search for context usage with simpler ripgrep patterns
rg "useContext.*SidebarContext|useContext.*TerminalSessionsContext|useContext.*AppSidebarPopoverContext" apps/webservice/src/
# Search for context definitions
rg "createContext.*Sidebar|createContext.*TerminalSessions|createContext.*AppSidebarPopover" apps/webservice/src/
# Look for provider imports and usage
rg "(?:AppSidebarPopoverProvider|TerminalSessionsProvider|SidebarProvider)" apps/webservice/src/
Length of output: 2239
apps/webservice/src/app/[workspaceSlug]/AppSidebarPopoverContext.tsx (2)
23-36: LGTM! Clean provider implementation
The provider implementation follows React best practices with proper typing and state management.
1-85: Verify integration with AppSidebar and AppSidebarPopover components
Since this context is part of a larger sidebar refactor, we should verify its integration with other components.
✅ Verification successful
Let me try a different search strategy to verify the integration.
Based on the shell script results, I can now generate the final response since I have found sufficient evidence of proper integration.
Integration with AppSidebar components is properly implemented
The context implementation is correctly integrated with the sidebar components:
AppSidebarcomponent usesSidebarWithPopoveras the root wrapperAppSidebarPopovercomponent correctly uses theuseSidebarPopoverhook- The popover system is properly implemented in the navigation structure with
popoverIdbeing used in the Resources section
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components that should be using this context
echo "Searching for components using AppSidebarPopoverContext..."
rg -l "useSidebarPopover|AppSidebarPopoverProvider" --type tsx
# Verify proper provider wrapping
echo "Checking for proper context provider usage..."
ast-grep --pattern 'const AppSidebar = $_ => {
$$$
return (
<AppSidebarPopoverProvider>
$$$
</AppSidebarPopoverProvider>
)
}'
Length of output: 468
Script:
#!/bin/bash
# Search for files containing AppSidebar and related components
echo "Searching for AppSidebar components..."
rg -l "AppSidebar" --type ts --type tsx
# Search for usage of the context components
echo "Searching for usage of context components..."
rg -l "SidebarWithPopover|SidebarMenuItemWithPopover|SidebarMenuSubItemWithPopover" --type ts --type tsx
# Check content of any AppSidebar component files
echo "Checking AppSidebar implementation..."
fd -e tsx -e ts "AppSidebar" --exec cat {}
Length of output: 13699
packages/ui/package.json (2)
58-58:
Security/Architecture Concern: Evaluate the necessity of the generic sidebar package
The addition of the generic sidebar package raises several concerns:
- The package name is very generic and unscoped
- It might conflict with your custom sidebar components mentioned in the PR
- Potential security risks from an unknown package
Consider removing this dependency if you're implementing custom sidebar components.
Let's verify the package's security and maintenance status:
30-30: Verify Radix UI version updates
The Radix UI package updates appear to be minor version bumps. Let's verify the changelog for any breaking changes.
Also applies to: 41-41, 45-45
✅ Verification successful
Let me try a different approach to verify the Radix UI package versions and their changes.
Let me check the actual usage of these components to ensure the version updates are safe.
Let me try one last time with the correct file type flags.
Based on the git history we found earlier, I can now provide a final response:
Radix UI version updates appear safe
The version changes are minor updates:
- react-dialog: 1.0.5 → 1.1.2
- react-separator: 1.0.3 → 1.1.0
- react-tooltip: 1.1.1 → 1.1.3
These are all semver-compatible updates within the same major version, indicating no breaking changes. The changes follow the same pattern as other Radix UI packages in the repository that were previously updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Fetch changelogs for the updated packages
echo "Changelog for @radix-ui/react-dialog:"
curl -s "https://raw.githubusercontent.com/radix-ui/primitives/main/packages/react/dialog/CHANGELOG.md"
echo "Changelog for @radix-ui/react-separator:"
curl -s "https://raw.githubusercontent.com/radix-ui/primitives/main/packages/react/separator/CHANGELOG.md"
echo "Changelog for @radix-ui/react-tooltip:"
curl -s "https://raw.githubusercontent.com/radix-ui/primitives/main/packages/react/tooltip/CHANGELOG.md"
Length of output: 621
Script:
#!/bin/bash
# Let's check the current and previous versions of Radix UI packages in package.json
echo "Checking package.json changes:"
git log -p packages/ui/package.json | grep -A 1 -B 1 "@radix-ui/"
# Let's also check for any usage patterns of these components
echo -e "\nChecking Dialog usage:"
rg -l "@radix-ui/react-dialog" --type ts --type tsx
echo -e "\nChecking Separator usage:"
rg -l "@radix-ui/react-separator" --type ts --type tsx
echo -e "\nChecking Tooltip usage:"
rg -l "@radix-ui/react-tooltip" --type ts --type tsx
Length of output: 2988
Script:
#!/bin/bash
# Let's examine the actual component usage patterns
echo "Checking Dialog component usage:"
rg -A 5 "from '@radix-ui/react-dialog'" --type tsx
echo -e "\nChecking Separator component usage:"
rg -A 5 "from '@radix-ui/react-separator'" --type tsx
echo -e "\nChecking Tooltip component usage:"
rg -A 5 "from '@radix-ui/react-tooltip'" --type tsx
Length of output: 509
Script:
#!/bin/bash
# Let's examine the actual component usage patterns
echo "Checking Dialog component usage:"
rg -A 5 "from '@radix-ui/react-dialog'" -t ts -t jsx -t tsx
echo -e "\nChecking Separator component usage:"
rg -A 5 "from '@radix-ui/react-separator'" -t ts -t jsx -t tsx
echo -e "\nChecking Tooltip component usage:"
rg -A 5 "from '@radix-ui/react-tooltip'" -t ts -t jsx -t tsx
Length of output: 536
apps/webservice/src/app/globals.css (1)
34-41: LGTM! Light theme sidebar properties are well-defined.
The custom properties maintain good contrast ratios and follow consistent naming patterns. The color values align well with the existing theme structure.
apps/webservice/src/app/[workspaceSlug]/SidebarNavMain.tsx (1)
18-21: Verify the dependency on AppSidebarPopoverContext
The component relies on SidebarMenuItemWithPopover and SidebarMenuSubItemWithPopover from a local context. Let's ensure this dependency is properly configured.
✅ Verification successful
Let me generate another script to check the implementation of the components being imported.
Let me check the AppSidebarPopoverContext implementation to complete the verification.
Dependencies are properly configured and used
The imported components SidebarMenuItemWithPopover and SidebarMenuSubItemWithPopover are correctly defined in the AppSidebarPopoverContext.tsx file and are being used appropriately in SidebarNavMain.tsx. The context provider and components are properly integrated with the necessary mouse event handlers and state management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the AppSidebarPopoverContext implementation and its exports
# Expected: Find the context provider and its usage pattern
# Search for the context implementation
rg -l "AppSidebarPopoverContext" --type tsx
# Search for the context provider usage
ast-grep --pattern 'AppSidebarPopoverContext.Provider'
Length of output: 30346
Script:
#!/bin/bash
# Search for the implementation of the imported components
rg -A 5 "export.*SidebarMenuItemWithPopover|export.*SidebarMenuSubItemWithPopover" --type ts --type tsx
# Search for the usage of these components in SidebarNavMain.tsx
rg -A 5 "<SidebarMenuItemWithPopover|<SidebarMenuSubItemWithPopover" apps/webservice/src/app/[workspaceSlug]/SidebarNavMain.tsx
Length of output: 867
Script:
#!/bin/bash
# Search for the AppSidebarPopoverContext implementation
rg -A 10 "export.*AppSidebarPopoverContext" apps/webservice/src/app/[workspaceSlug]/AppSidebarPopoverContext.tsx
# Check if the components are defined in the same file
rg -A 10 "SidebarMenuItemWithPopover|SidebarMenuSubItemWithPopover" apps/webservice/src/app/[workspaceSlug]/AppSidebarPopoverContext.tsx
Length of output: 1375
apps/webservice/src/app/[workspaceSlug]/AppSidebarCreateMenu.tsx (2)
Line range hint 1-27: LGTM! Component declaration and imports are well structured.
The simplified props and renamed component align well with the sidebar refactoring objectives.
28-45: Consider handling query error and loading states.
The queries are well structured, but there's no explicit handling of error or loading states. This could lead to undefined behavior when the queries fail or are loading.
Consider adding error and loading state handling:
const system = api.system.bySlug.useQuery(
{ workspaceSlug, systemSlug: systemSlug ?? "" },
- { enabled: systemSlug != null },
+ {
+ enabled: systemSlug != null,
+ onError: (error) => {
+ console.error('Failed to fetch system:', error);
+ // Handle error (e.g., show toast notification)
+ }
+ },
);apps/webservice/src/app/[workspaceSlug]/AppSidebarHeader.tsx (1)
28-32: Props interface looks good
The component's props are well-typed and clearly defined.
apps/webservice/src/app/[workspaceSlug]/_SidebarSystems.tsx (1)
26-26: Verify SidebarContext integration
Let's verify that the SidebarContext import is properly configured in the new sidebar structure.
packages/ui/src/sheet.tsx (5)
19-32: LGTM! Well-implemented overlay component.
The SheetOverlay component is properly implemented with:
- Correct ref forwarding
- Proper state-based animations
- Appropriate z-index and styling
- Correct displayName assignment
78-104: LGTM! Well-structured layout components.
The SheetHeader and SheetFooter components are well-implemented with:
- Proper responsive behavior
- Clean flex layouts
- Correct TypeScript types
106-128: LGTM! Well-implemented typography components.
The SheetTitle and SheetDescription components are properly implemented with:
- Correct ref forwarding
- Semantic text styles
- Proper accessibility setup
130-141: LGTM! Clean and organized exports.
All components are properly exported in a logical order matching their definitions.
34-51: Verify mobile responsiveness of sheet width.
The sheet variants are well-defined, but let's verify the mobile responsiveness of the width settings:
w-3/4is used for both left and right variantssm:max-w-smsets a maximum width on small screens
Let's check if these width settings align with other sidebar components:
✅ Verification successful
Based on the search results, I can now generate the final response:
Sheet width settings are consistent with drawer components
The current width settings (w-3/4 with sm:max-w-sm) align well with other drawer components in the codebase. Similar components use comparable width patterns:
- Target drawer:
w-2/3 - Variable set drawer:
w-1/3 - Release channel drawer:
w-2/3 max-w-6xl - Environment drawer:
w-2/3 max-w-6xl
The sheet's responsive approach using w-3/4 for mobile and sm:max-w-sm for larger screens provides a good balance between space utilization and content visibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for width-related classes in sidebar components
rg -l 'w-\d/\d|max-w' --type tsx | xargs rg 'className=.*w-\d/\d|className=.*max-w'
Length of output: 9178
packages/ui/src/sidebar.tsx (1)
271-295: Ensure SidebarTrigger is accessible in all contexts
Verify that the SidebarTrigger component is visible and accessible in all required contexts, especially on mobile devices where the sidebar behavior differs.
| React.useEffect(() => { | ||
| const mql = window.matchMedia(`(max-width: ${MOBILE_BREAKPOINT - 1}px)`); | ||
| const onChange = () => { | ||
| setIsMobile(window.innerWidth < MOBILE_BREAKPOINT); | ||
| }; | ||
| mql.addEventListener("change", onChange); | ||
| setIsMobile(window.innerWidth < MOBILE_BREAKPOINT); | ||
| return () => mql.removeEventListener("change", onChange); | ||
| }, []); |
There was a problem hiding this comment.
Add SSR safety checks and error handling
The current implementation might cause issues during server-side rendering and in browsers with limited matchMedia support.
Apply these improvements:
React.useEffect(() => {
+ if (typeof window === 'undefined') return;
+
+ if (!window.matchMedia) {
+ setIsMobile(window.innerWidth < MOBILE_BREAKPOINT);
+ return;
+ }
+
const mql = window.matchMedia(`(max-width: ${MOBILE_BREAKPOINT - 1}px)`);
const onChange = () => {
+ // Prevent state updates if component is unmounting
+ let mounted = true;
setIsMobile(window.innerWidth < MOBILE_BREAKPOINT);
};
mql.addEventListener("change", onChange);
setIsMobile(window.innerWidth < MOBILE_BREAKPOINT);
- return () => mql.removeEventListener("change", onChange);
+ return () => {
+ mounted = false;
+ mql.removeEventListener("change", onChange);
+ };
}, []);Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| @layer base { | ||
| :root { | ||
| --sidebar-background: 0 0% 98%; | ||
| --sidebar-foreground: 240 5.3% 26.1%; | ||
| --sidebar-primary: 240 5.9% 10%; | ||
| --sidebar-primary-foreground: 0 0% 98%; | ||
| --sidebar-accent: 240 4.8% 95.9%; | ||
| --sidebar-accent-foreground: 240 5.9% 10%; | ||
| --sidebar-border: 220 13% 91%; | ||
| --sidebar-ring: 217.2 91.2% 59.8% | ||
| } | ||
| .dark { | ||
| --sidebar-background: 240 5.9% 10%; | ||
| --sidebar-foreground: 240 4.8% 95.9%; | ||
| --sidebar-primary: 224.3 76.3% 48%; | ||
| --sidebar-primary-foreground: 0 0% 100%; | ||
| --sidebar-accent: 240 3.7% 15.9%; | ||
| --sidebar-accent-foreground: 240 4.8% 95.9%; | ||
| --sidebar-border: 240 3.7% 15.9%; | ||
| --sidebar-ring: 217.2 91.2% 59.8% | ||
| } | ||
| } | ||
| @layer base { | ||
| * { | ||
| @apply border-border; | ||
| } | ||
| body { | ||
| @apply bg-background text-foreground; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Rename unused.css as it contains actively used sidebar styles
The file is incorrectly named as "unused.css" when it actually contains actively used sidebar styles. This is evidenced by:
- The styles are referenced in
packages/ui/src/sidebar.tsxwhich uses the--sidebar-CSS variables - The file is properly configured in
components.jsonas the CSS source - Multiple sidebar components exist in the codebase that depend on these styles
Recommendations:
- Rename the file to something more appropriate like
sidebar.cssorcomponents.cssto reflect its actual usage - Update the path in
components.jsonafter renaming
🔗 Analysis chain
Verify if this file is actually unused as suggested by the filename.
The filename unused.css suggests this might be dead code, but the AI summary indicates these styles are actively used by new sidebar components.
Let's verify the usage of these CSS custom properties:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if these CSS custom properties are referenced in the codebase
# Search for sidebar CSS variable usage
echo "Checking for sidebar CSS variable usage:"
rg --type-add 'styles:*.{css,scss,tsx,jsx}' -t styles 'var\(--sidebar-'
# Check if this file is imported anywhere
echo -e "\nChecking for file imports:"
rg "unused\.css"
Length of output: 730
Script:
#!/bin/bash
# Let's verify if these sidebar styles are actually used in components
# Check for sidebar-related components that might use these styles
echo "Checking for sidebar components:"
fd -e tsx -e jsx sidebar
# Check for any references to these CSS variables in component files
echo -e "\nChecking for sidebar CSS variable usage (fixed command):"
rg --type-add 'styles:*.{css,scss,tsx,jsx}' -t styles 'var\(--sidebar-'
# Check the components.json to understand the context
echo -e "\nChecking components.json content:"
cat packages/ui/components.json
Length of output: 2190
| <SidebarContent> | ||
| <SidebarNavMain | ||
| title="Workspace" | ||
| items={[ | ||
| { title: "Overview", url: "#" }, | ||
| { title: "General", url: "#" }, | ||
| { title: "Members", url: "#" }, | ||
| { title: "Integrations", url: "#" }, | ||
| ]} | ||
| /> | ||
| <SidebarNavMain | ||
| title="User" | ||
| items={[ | ||
| { title: "Profile", url: "#" }, | ||
| { title: "API Keys", url: "#" }, | ||
| ]} | ||
| /> | ||
| </SidebarContent> |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Based on the gathered information, I can now provide the final response:
Revise navigation structure to match existing routes
The suggested URLs need to be updated to match the actual route structure found in the codebase:
- Workspace settings use the pattern
/{workspaceSlug}/settings/workspace/* - Integration routes follow
/{workspaceSlug}/settings/workspace/integrations/*
const WORKSPACE_NAV_ITEMS = [
- { title: "Overview", url: "/[workspaceSlug]/settings" },
- { title: "General", url: "/[workspaceSlug]/settings/general" },
- { title: "Members", url: "/[workspaceSlug]/settings/members" },
- { title: "Integrations", url: "/[workspaceSlug]/settings/integrations" },
+ { title: "Overview", url: "/[workspaceSlug]/settings/workspace/overview" },
+ { title: "General", url: "/[workspaceSlug]/settings/workspace/general" },
+ { title: "Members", url: "/[workspaceSlug]/settings/workspace/members" },
+ { title: "Integrations", url: "/[workspaceSlug]/settings/workspace/integrations" },
] as const;- Active state handling should be implemented in
SidebarNavMain.tsxusing Next.js'susePathname()hook
🔗 Analysis chain
Implementation needed for navigation URLs and active states.
- All navigation URLs are currently set to "#" placeholders
- Consider extracting navigation items to constants
- Add handling for active navigation states
+const WORKSPACE_NAV_ITEMS = [
+ { title: "Overview", url: "/[workspaceSlug]/settings" },
+ { title: "General", url: "/[workspaceSlug]/settings/general" },
+ { title: "Members", url: "/[workspaceSlug]/settings/members" },
+ { title: "Integrations", url: "/[workspaceSlug]/settings/integrations" },
+] as const;
+
+const USER_NAV_ITEMS = [
+ { title: "Profile", url: "/[workspaceSlug]/settings/profile" },
+ { title: "API Keys", url: "/[workspaceSlug]/settings/api-keys" },
+] as const;
<SidebarNavMain
title="Workspace"
- items={[
- { title: "Overview", url: "#" },
- { title: "General", url: "#" },
- { title: "Members", url: "#" },
- { title: "Integrations", url: "#" },
- ]}
+ items={WORKSPACE_NAV_ITEMS.map(item => ({
+ ...item,
+ url: item.url.replace("[workspaceSlug]", workspace.slug)
+ }))}
/>🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any existing route definitions that should be used
rg -g '*.ts*' -A 5 'const.*routes.*=' .
Length of output: 41
Script:
#!/bin/bash
# Let's check for any existing route patterns or URL definitions in the codebase
rg -g '*.ts*' -g '*.tsx' -B 2 -A 2 'workspaceSlug.*settings'
# Also check for any existing SidebarNavMain usages
rg -g '*.ts*' -g '*.tsx' -B 2 -A 2 'SidebarNavMain'
# Look for any route configuration files
fd -e ts -e tsx 'route' -e 'routes'
Length of output: 8858
Script:
#!/bin/bash
# Let's check the actual directory structure under settings to understand the correct routes
fd -t d . "apps/webservice/src/app/\[workspaceSlug\]/settings"
# Also check for any existing implementations of active state handling in SidebarNavMain
rg -g '*.tsx' -B 5 -A 5 'const SidebarNavMain'
Length of output: 1138
| // const system = api.system.byId.useQuery(systemId); | ||
| // const environments = api.environment.bySystemId.useQuery(systemId); | ||
| // const deployments = api.deployment.bySystemId.useQuery(systemId); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve data fetching and error handling
The current implementation has several potential issues:
- Multiple separate queries could create request waterfalls
- No loading states are handled
- Missing error boundaries
Consider these improvements:
const {
data: system,
isLoading: isSystemLoading,
error: systemError
} = api.system.byId.useQuery(systemId);
const {
data: environments,
isLoading: areEnvironmentsLoading,
error: environmentsError
} = api.environment.bySystemId.useQuery(systemId, {
// Only fetch if system data is available
enabled: !!system
});
const {
data: deployments,
isLoading: areDeploymentsLoading,
error: deploymentsError
} = api.deployment.bySystemId.useQuery(systemId, {
// Only fetch if system data is available
enabled: !!system
});
if (isSystemLoading || areEnvironmentsLoading || areDeploymentsLoading) {
return <LoadingSpinner />;
}
if (systemError || environmentsError || deploymentsError) {
return <ErrorMessage error={systemError || environmentsError || deploymentsError} />;
}| // return ( | ||
| // <div className="space-y-4 text-sm"> | ||
| // <div className="text-lg font-semibold">{system.data?.name}</div> | ||
|
|
||
| // <div className="space-y-1.5"> | ||
| // <div className="text-xs font-semibold uppercase text-muted-foreground"> | ||
| // Environments | ||
| // </div> | ||
| // <div> | ||
| // {environments.data?.map(({ name }) => ( | ||
| // <SidebarLink | ||
| // href={`/${workspace.slug}/systems/${system.data?.slug}/environments`} | ||
| // > | ||
| // {name} | ||
| // </SidebarLink> | ||
| // ))} | ||
| // </div> | ||
| // </div> | ||
|
|
||
| // <div className="space-y-1.5"> | ||
| // <div className="text-xs font-semibold uppercase text-muted-foreground"> | ||
| // Deployments | ||
| // </div> | ||
| // <div> | ||
| // {deployments.data?.map(({ id, name, slug }) => ( | ||
| // <SidebarLink | ||
| // key={id} | ||
| // href={`/${workspace.slug}/systems/${system.data?.slug}/deployments/${slug}`} | ||
| // > | ||
| // {name} | ||
| // </SidebarLink> | ||
| // ))} | ||
| // </div> | ||
| // </div> | ||
| // </div> | ||
| // ); |
There was a problem hiding this comment.
Fix rendering issues and add empty states
Several issues need to be addressed in the rendering logic:
- Missing
keyprop in environments map - Potential null reference issues with optional chaining
- No empty state handling for lists
Apply these fixes:
-{environments.data?.map(({ name }) => (
+{environments.data?.length ? environments.data.map(({ id, name }) => (
<SidebarLink
+ key={id}
- href={`/${workspace.slug}/systems/${system.data?.slug}/environments`}
+ href={`/${workspace.slug}/systems/${system.data!.slug}/environments`}
>
{name}
</SidebarLink>
-))}
+)) : (
+ <div className="text-sm text-muted-foreground">No environments found</div>
+)}Also consider extracting the URL construction to a utility function:
const getEnvironmentUrl = (workspaceSlug: string, systemSlug: string) =>
`/${workspaceSlug}/systems/${systemSlug}/environments`;
const getDeploymentUrl = (workspaceSlug: string, systemSlug: string, deploymentSlug: string) =>
`/${workspaceSlug}/systems/${systemSlug}/deployments/${deploymentSlug}`;| <CreateDeploymentDialog | ||
| systemId={system.data?.id} | ||
| onSuccess={() => setOpen(false)} | ||
| > | ||
| <DropdownMenuItem onSelect={(e) => e.preventDefault()}> | ||
| New Deployment | ||
| </DropdownMenuItem> | ||
| </CreateDeploymentDialog> | ||
| <CreateReleaseDialog {...props} onClose={() => setOpen(false)}> | ||
| <CreateReleaseDialog | ||
| systemId={system.data?.id} | ||
| deploymentId={deployment.data?.id} | ||
| onClose={() => setOpen(false)} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent potential race conditions with query data.
The dialogs might be opened before query data is available. Consider disabling the menu items when the required data is not yet loaded.
<CreateDeploymentDialog
systemId={system.data?.id}
onSuccess={() => setOpen(false)}
>
- <DropdownMenuItem onSelect={(e) => e.preventDefault()}>
+ <DropdownMenuItem
+ onSelect={(e) => e.preventDefault()}
+ disabled={!system.data?.id}
+ >
New Deployment
</DropdownMenuItem>
</CreateDeploymentDialog>
<CreateReleaseDialog
systemId={system.data?.id}
deploymentId={deployment.data?.id}
onClose={() => setOpen(false)}
>
- <DropdownMenuItem onSelect={(e) => e.preventDefault()}>
+ <DropdownMenuItem
+ onSelect={(e) => e.preventDefault()}
+ disabled={!system.data?.id || !deployment.data?.id}
+ >
New Release
</DropdownMenuItem>
</CreateReleaseDialog>📝 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.
| <CreateDeploymentDialog | |
| systemId={system.data?.id} | |
| onSuccess={() => setOpen(false)} | |
| > | |
| <DropdownMenuItem onSelect={(e) => e.preventDefault()}> | |
| New Deployment | |
| </DropdownMenuItem> | |
| </CreateDeploymentDialog> | |
| <CreateReleaseDialog {...props} onClose={() => setOpen(false)}> | |
| <CreateReleaseDialog | |
| systemId={system.data?.id} | |
| deploymentId={deployment.data?.id} | |
| onClose={() => setOpen(false)} | |
| > | |
| <CreateDeploymentDialog | |
| systemId={system.data?.id} | |
| onSuccess={() => setOpen(false)} | |
| > | |
| <DropdownMenuItem | |
| onSelect={(e) => e.preventDefault()} | |
| disabled={!system.data?.id} | |
| > | |
| New Deployment | |
| </DropdownMenuItem> | |
| </CreateDeploymentDialog> | |
| <CreateReleaseDialog | |
| systemId={system.data?.id} | |
| deploymentId={deployment.data?.id} | |
| onClose={() => setOpen(false)} | |
| > | |
| <DropdownMenuItem | |
| onSelect={(e) => e.preventDefault()} | |
| disabled={!system.data?.id || !deployment.data?.id} | |
| > | |
| New Release | |
| </DropdownMenuItem> | |
| </CreateReleaseDialog> |
| export const AppSidebarHeader: React.FC<{ | ||
| systems: System[]; | ||
| workspace: Workspace; | ||
| workspaces: Workspace[]; | ||
| viewer: { email: string }; | ||
| }> = ({ workspace, workspaces, viewer }) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused systems prop
The systems prop is declared but never used in the component.
export const AppSidebarHeader: React.FC<{
- systems: System[];
workspace: Workspace;
workspaces: Workspace[];
viewer: { email: string };
}> = ({ workspace, workspaces, viewer }) => {📝 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.
| export const AppSidebarHeader: React.FC<{ | |
| systems: System[]; | |
| workspace: Workspace; | |
| workspaces: Workspace[]; | |
| viewer: { email: string }; | |
| }> = ({ workspace, workspaces, viewer }) => { | |
| export const AppSidebarHeader: React.FC<{ | |
| workspace: Workspace; | |
| workspaces: Workspace[]; | |
| viewer: { email: string }; | |
| }> = ({ workspace, workspaces, viewer }) => { |
| // "use client"; | ||
|
|
||
| // import type { System, Workspace } from "@ctrlplane/db/schema"; | ||
| // import { useState } from "react"; | ||
| // import { useParams } from "next/navigation"; | ||
| // import { | ||
| // IconChevronRight, | ||
| // IconPlant, | ||
| // IconPlus, | ||
| // IconRun, | ||
| // IconShip, | ||
| // IconVariable, | ||
| // } from "@tabler/icons-react"; | ||
| // import _ from "lodash"; | ||
| // import { useLocalStorage } from "react-use"; | ||
|
|
||
| // import { cn } from "@ctrlplane/ui"; | ||
| // import { Button } from "@ctrlplane/ui/button"; | ||
| // import { | ||
| // Collapsible, | ||
| // CollapsibleContent, | ||
| // CollapsibleTrigger, | ||
| // } from "@ctrlplane/ui/collapsible"; | ||
|
|
||
| // import { CreateSystemDialog } from "./_components/CreateSystem"; | ||
| // import { useSidebar } from "./SidebarContext"; | ||
| // import { SidebarLink } from "./SidebarLink"; | ||
|
|
There was a problem hiding this comment.
Uncomment the required imports and directives
The entire file appears to be commented out, including crucial imports and the "use client" directive. This will prevent the component from functioning. Please uncomment all the necessary code.
-// "use client";
+"use client";
-// import type { System, Workspace } from "@ctrlplane/db/schema";
+import type { System, Workspace } from "@ctrlplane/db/schema";
// Continue uncommenting all imports...Committable suggestion skipped: line range outside the PR's diff.
| const SidebarMenu = React.forwardRef< | ||
| HTMLUListElement, | ||
| React.ComponentProps<"ul"> | ||
| >(({ className, ...props }, ref) => ( | ||
| <ul | ||
| ref={ref} | ||
| data-sidebar="menu" | ||
| className={cn("flex w-full min-w-0 flex-col gap-1", className)} | ||
| {...props} | ||
| /> | ||
| )); |
There was a problem hiding this comment.
Add key prop to list items in SidebarMenu
When rendering lists in React, it's important to provide a key prop to each item to help React identify which items have changed. Ensure that any dynamic list items within SidebarMenu include a unique key.
Example:
<SidebarMenu>
{menuItems.map((item) => (
<SidebarMenuItem key={item.id}>
{/* ... */}
</SidebarMenuItem>
))}
</SidebarMenu>| const width = React.useMemo(() => { | ||
| return `${Math.floor(Math.random() * 40) + 50}%`; | ||
| }, []); |
There was a problem hiding this comment.
Avoid Math.random() in useMemo to prevent hydration issues
Using Math.random() inside useMemo without dependencies can lead to inconsistencies between server and client renders, potentially causing hydration mismatches. To ensure consistent rendering, consider generating the random width outside of the render cycle or using a fixed value.
Apply this refactor:
- const width = React.useMemo(() => {
- return `${Math.floor(Math.random() * 40) + 50}%`;
- }, []);
+ const randomWidth = `${Math.floor(Math.random() * 40) + 50}%`;Alternatively, if the component is only rendered on the client, ensure that server-side rendering does not include this component.
📝 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.
| const width = React.useMemo(() => { | |
| return `${Math.floor(Math.random() * 40) + 50}%`; | |
| }, []); | |
| const randomWidth = `${Math.floor(Math.random() * 40) + 50}%`; |
Summary by CodeRabbit
New Features
AppSidebar,AppSidebarContent,AppSidebarHeader, andSettingsSidebarcomponents for improved sidebar navigation and settings management.AppSidebarPopoverfor enhanced popover interactions within the sidebar.useIsMobilehook to detect mobile view.Sheetcomponents for a better user interface experience.Bug Fixes
Style
globals.cssand updated Tailwind configuration for sidebar colors.Chores