[WEB-5525] improvement: update WorkspaceMenuRoot to use variant prop for layout adjustments #8196
Conversation
…for layout adjustments
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
WalkthroughThis change refactors the sidebar and workspace menu system by replacing a boolean flag with a variant-based prop ("sidebar" | "top-navigation"), introduces new desktop-specific components for conditional sidebar rendering, removes the deprecated SidebarDropdown component, and updates related navigation integration points. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/core/components/workspace/sidebar/workspace-menu-root.tsx (1)
138-138: Fix typo in Transition class.There's a typo in the
enterToclass name:- enterTo="trnsform opacity-100 scale-100" + enterTo="transform opacity-100 scale-100"
🧹 Nitpick comments (3)
apps/web/ce/components/desktop/helper.ts (1)
1-1: Consider adding explicit return type annotation.For better documentation and type safety, consider adding an explicit return type:
-export const isSidebarToggleVisible = () => true; +export const isSidebarToggleVisible = (): boolean => true;apps/web/ce/components/desktop/sidebar-workspace-menu.tsx (1)
1-3: Consider adding return type and JSDoc for clarity.Since this is a CE placeholder (likely overridden in EE), adding a brief comment and explicit return type improves maintainability:
+/** + * CE placeholder for desktop sidebar workspace menu. + * Returns null - actual implementation provided in EE. + */ -export function DesktopSidebarWorkspaceMenu() { +export function DesktopSidebarWorkspaceMenu(): React.ReactNode { return null; }apps/web/core/components/workspace/sidebar/workspace-menu-root.tsx (1)
79-83: State synchronization within render may cause issues.Setting state directly within the render function's render prop callback can lead to extra re-renders. While this pattern works, consider using
useEffectto sync the open state:- {({ open, close }: { open: boolean; close: () => void }) => { - // Update local state directly - if (isWorkspaceMenuOpen !== open) { - setIsWorkspaceMenuOpen(open); - } + {({ open, close }: { open: boolean; close: () => void }) => { + // Sync handled via useEffect belowAnd add a separate effect:
useEffect(() => { // This would require accessing `open` from Menu, which isn't straightforward // Current approach works but consider refactoring if re-render issues arise }, []);Given Headless UI's design, the current approach is acceptable but worth monitoring for performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/web/ce/components/desktop/helper.ts(1 hunks)apps/web/ce/components/desktop/index.ts(1 hunks)apps/web/ce/components/desktop/sidebar-workspace-menu.tsx(1 hunks)apps/web/ce/components/navigations/top-navigation-root.tsx(1 hunks)apps/web/core/components/navigation/app-rail-root.tsx(2 hunks)apps/web/core/components/sidebar/sidebar-toggle-button.tsx(1 hunks)apps/web/core/components/workspace/sidebar/dropdown.tsx(0 hunks)apps/web/core/components/workspace/sidebar/workspace-menu-root.tsx(5 hunks)
💤 Files with no reviewable changes (1)
- apps/web/core/components/workspace/sidebar/dropdown.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,mts,cts}
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.{ts,tsx,mts,cts}: Useconsttype parameters for more precise literal inference in TypeScript 5.0+
Use thesatisfiesoperator to validate types without widening them
Leverage inferred type predicates to reduce the need for explicitisreturn types in filter/check functions
UseNoInfer<T>utility to block inference for specific type arguments when they should be determined by other arguments
Utilize narrowing inswitch(true)blocks for control flow analysis (TypeScript 5.3+)
Rely on narrowing from direct boolean comparisons for type guards
Trust preserved narrowing in closures when variables aren't modified after the check (TypeScript 5.4+)
Use constant indices to narrow object/array properties (TypeScript 5.5+)
Use standard ECMAScript decorators (Stage 3) instead of legacyexperimentalDecorators
Useusingdeclarations for explicit resource management with Disposable pattern instead of manual cleanup (TypeScript 5.2+)
Usewith { type: "json" }for import attributes; avoid deprecatedassertsyntax (TypeScript 5.3/5.8+)
Useimport typeexplicitly when importing types to ensure they are erased during compilation, respectingverbatimModuleSyntaxflag
Use.ts,.mts,.ctsextensions inimport typestatements (TypeScript 5.2+)
Useimport type { Type } from "mod" with { "resolution-mode": "import" }for specific module resolution contexts (TypeScript 5.3+)
Use new iterator methods (map, filter, etc.) if targeting modern environments (TypeScript 5.6+)
Utilize newSetmethods likeunion,intersection, etc., when available (TypeScript 5.5+)
UseObject.groupBy/Map.groupBystandard methods for grouping instead of external libraries (TypeScript 5.4+)
UsePromise.withResolvers()for creating promises with exposed resolve/reject functions (TypeScript 5.7+)
Use copying array methods (toSorted,toSpliced,with) for immutable array operations (TypeScript 5.2+)
Avoid accessing instance fields viasuperin classes (TypeScript 5....
Files:
apps/web/ce/components/desktop/index.tsapps/web/ce/components/navigations/top-navigation-root.tsxapps/web/core/components/sidebar/sidebar-toggle-button.tsxapps/web/ce/components/desktop/helper.tsapps/web/core/components/navigation/app-rail-root.tsxapps/web/ce/components/desktop/sidebar-workspace-menu.tsxapps/web/core/components/workspace/sidebar/workspace-menu-root.tsx
🧠 Learnings (2)
📚 Learning: 2025-10-21T17:22:05.204Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7989
File: apps/web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/[pageId]/page.tsx:45-46
Timestamp: 2025-10-21T17:22:05.204Z
Learning: In the makeplane/plane repository, the refactor from useParams() to params prop is specifically scoped to page.tsx and layout.tsx files in apps/web/app (Next.js App Router pattern). Other components (hooks, regular client components, utilities) should continue using the useParams() hook as that is the correct pattern for non-route components.
Applied to files:
apps/web/core/components/navigation/app-rail-root.tsx
📚 Learning: 2025-09-02T08:14:49.260Z
Learnt from: sriramveeraghanta
Repo: makeplane/plane PR: 7697
File: apps/web/app/(all)/[workspaceSlug]/(projects)/header.tsx:12-13
Timestamp: 2025-09-02T08:14:49.260Z
Learning: The star-us-link.tsx file in apps/web/app/(all)/[workspaceSlug]/(projects)/ already has "use client" directive at the top, making it a proper Client Component for hook usage.
Applied to files:
apps/web/core/components/navigation/app-rail-root.tsx
🧬 Code graph analysis (4)
apps/web/ce/components/navigations/top-navigation-root.tsx (1)
apps/web/core/components/workspace/sidebar/workspace-menu-root.tsx (1)
WorkspaceMenuRoot(31-228)
apps/web/core/components/sidebar/sidebar-toggle-button.tsx (1)
apps/web/ce/components/desktop/helper.ts (1)
isSidebarToggleVisible(1-1)
apps/web/core/components/navigation/app-rail-root.tsx (1)
apps/web/ce/components/desktop/sidebar-workspace-menu.tsx (1)
DesktopSidebarWorkspaceMenu(1-3)
apps/web/core/components/workspace/sidebar/workspace-menu-root.tsx (1)
apps/web/core/components/workspace/logo.tsx (1)
WorkspaceLogo(12-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
apps/web/core/components/navigation/app-rail-root.tsx (1)
11-12: LGTM!The import and placement of
DesktopSidebarWorkspaceMenuare correct. The component serves as a placeholder for future desktop-specific workspace menu functionality while currently rendering nothing (returnsnull).Also applies to: 44-44
apps/web/ce/components/navigations/top-navigation-root.tsx (1)
23-23: LGTM!Correct usage of the new
variantprop. The"top-navigation"value properly indicates this menu instance should render with the top navigation layout styling.apps/web/ce/components/desktop/index.ts (1)
1-2: LGTM!Standard barrel exports for the new desktop module. This properly exposes
isSidebarToggleVisibleandDesktopSidebarWorkspaceMenufor consumption by other modules.apps/web/core/components/sidebar/sidebar-toggle-button.tsx (1)
5-5: LGTM!Clean implementation of conditional rendering based on the visibility helper. The early return pattern is appropriate and allows edition-specific (CE/EE) control over the toggle button visibility without modifying this core component.
Also applies to: 11-11
apps/web/core/components/workspace/sidebar/workspace-menu-root.tsx (2)
27-32: LGTM on the variant prop refactor.The change from
renderLogoOnly?: booleantovariant: "sidebar" | "top-navigation"improves clarity and extensibility. The discriminated union approach makes the intent explicit and is more maintainable than boolean flags.
144-151: Verify fixed positioning works across different viewport configurations.The dropdown uses fixed positioning with hardcoded offsets (
top-11 left-14for sidebar,top-10 left-4for top-navigation). Ensure these values align correctly when:
- The sidebar rail width changes based on
showLabelpreference- Different screen sizes or zoom levels are used
Description
Type of Change
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.