[WEB-5556] chore: tab navigation and sidebar improvements #8218
Conversation
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRefactors responsive tab layout to use a ResizeObserver-driven callback ref for container measurement and updates destructuring order in the tab navigation root. Also closes the extended project sidebar when a project list item is clicked. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,mts,cts}📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
Files:
🧠 Learnings (1)📚 Learning: 2025-10-21T17:22:05.204ZApplied to files:
🧬 Code graph analysis (1)apps/web/core/components/navigation/use-responsive-tab-layout.ts (1)
⏰ 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)
🔇 Additional comments (2)
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 |
There was a problem hiding this comment.
Pull request overview
This PR improves sidebar navigation and tab responsiveness by refactoring the ResizeObserver implementation and adding extended project sidebar auto-close functionality.
- Refactored
useResponsiveTabLayoutto use callback ref pattern instead of traditional useRef for better ResizeObserver lifecycle management - Added auto-close behavior for the extended project sidebar when navigating to a project
- Reorganized return values and destructuring for consistency
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
apps/web/core/components/workspace/sidebar/projects-list-item.tsx |
Adds extended project sidebar auto-close when clicking project items |
apps/web/core/components/navigation/use-responsive-tab-layout.ts |
Refactors ResizeObserver from useEffect to callback ref pattern for immediate measurement |
apps/web/core/components/navigation/tab-navigation-root.tsx |
Updates destructuring order to match refactored hook return value |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/web/core/components/workspace/sidebar/projects-list-item.tsx
Outdated
Show resolved
Hide resolved
| const containerRef = useCallback((node: HTMLDivElement | null) => { | ||
| // Clean up previous observer if it exists | ||
| if (resizeObserverRef.current) { | ||
| resizeObserverRef.current.disconnect(); | ||
| resizeObserverRef.current = null; | ||
| } | ||
|
|
||
| // ResizeObserver to measure container width | ||
| useEffect(() => { | ||
| if (!container) return; | ||
| // If node is null (unmounting), just clean up | ||
| if (!node) { | ||
| setContainerWidth(0); | ||
| return; | ||
| } | ||
|
|
||
| // Set initial width immediately | ||
| setContainerWidth(node.offsetWidth); | ||
|
|
||
| // Create and set up new ResizeObserver | ||
| const resizeObserver = new ResizeObserver((entries) => { | ||
| for (const entry of entries) { | ||
| setContainerWidth(entry.contentRect.width); | ||
| } | ||
| }); | ||
|
|
||
| resizeObserver.observe(container); | ||
|
|
||
| return () => { | ||
| resizeObserver.disconnect(); | ||
| }; | ||
| }, [container]); | ||
| resizeObserverRef.current = resizeObserver; | ||
| resizeObserver.observe(node); | ||
| }, []); // Empty deps - callback function remains stable |
There was a problem hiding this comment.
The ResizeObserver lacks proper cleanup on component unmount. If the component unmounts while a ResizeObserver callback is pending, it may attempt to call setContainerWidth on an unmounted component, potentially causing a memory leak or React warning.
Add a cleanup effect to ensure the observer is disconnected when the component unmounts:
useEffect(() => {
return () => {
if (resizeObserverRef.current) {
resizeObserverRef.current.disconnect();
resizeObserverRef.current = null;
}
};
}, []);![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
🧹 Nitpick comments (3)
apps/web/core/components/workspace/sidebar/projects-list-item.tsx (2)
71-75: Consolidate duplicateuseAppThemecallsYou’re calling
useAppTheme()twice; you can grab all three values in a single call to avoid redundant hook invocations and keep related theme concerns together.- const { toggleAnySidebarDropdown } = useAppTheme(); + const { toggleAnySidebarDropdown, isExtendedProjectSidebarOpened, toggleExtendedProjectSidebar } = useAppTheme(); - const { isExtendedProjectSidebarOpened, toggleExtendedProjectSidebar } = useAppTheme();
257-267: Confirm whether accordion-only toggles should also close the extended sidebar
handleItemClicknow closes the extended project sidebar even whennavigationMode === "accordion"and we’re just expanding/collapsing the list (norouter.pushin that branch). If the intent is to close the extended sidebar only after actual navigation, consider moving this logic into the non-accordion branch:const handleItemClick = () => { if (projectPreferences.navigationMode === "accordion") { setIsProjectListOpen(!isProjectListOpen); } else { router.push(defaultTabUrl); + if (isExtendedProjectSidebarOpened) { + toggleExtendedProjectSidebar(false); + } } - // close the extended sidebar if it is open - if (isExtendedProjectSidebarOpened) { - toggleExtendedProjectSidebar(false); - } };apps/web/core/components/navigation/use-responsive-tab-layout.ts (1)
4-10: ResizeObserver-basedcontainerReflooks correct; consider minor typing/compat tweaksThe new
containerRefcallback +resizeObserverRefpattern correctly:
- Disconnects any existing observer before attaching a new one.
- Resets
containerWidthonnullref.- Initializes width immediately and reacts to subsequent
ResizeObserverentries.Two optional refinements you might consider:
- Use the built-in ref callback type for clarity
-export type TResponsiveTabLayout = { +export type TResponsiveTabLayout = { visibleItems: TNavigationItem[]; overflowItems: TNavigationItem[]; hasOverflow: boolean; itemRefs: React.MutableRefObject<(HTMLDivElement | null)[]>; - containerRef: (node: HTMLDivElement | null) => void; + containerRef: React.RefCallback<HTMLDivElement>; };
- Guard for environments without
ResizeObserver(if you support them / SSR usage is possible)- const containerRef = useCallback((node: HTMLDivElement | null) => { + const containerRef = useCallback((node: HTMLDivElement | null) => { + if (typeof ResizeObserver === "undefined") { + if (node) setContainerWidth(node.offsetWidth); + else setContainerWidth(0); + return; + }These are not blockers; the current implementation is sound for modern browsers and client-only usage.
Also applies to: 33-71
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/core/components/navigation/tab-navigation-root.tsx(1 hunks)apps/web/core/components/navigation/use-responsive-tab-layout.ts(4 hunks)apps/web/core/components/workspace/sidebar/projects-list-item.tsx(2 hunks)
🧰 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/core/components/navigation/tab-navigation-root.tsxapps/web/core/components/navigation/use-responsive-tab-layout.tsapps/web/core/components/workspace/sidebar/projects-list-item.tsx
🧠 Learnings (1)
📚 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/use-responsive-tab-layout.ts
🧬 Code graph analysis (2)
apps/web/core/components/navigation/tab-navigation-root.tsx (1)
apps/web/core/components/navigation/use-responsive-tab-layout.ts (1)
useResponsiveTabLayout(28-152)
apps/web/core/components/navigation/use-responsive-tab-layout.ts (1)
apps/web/core/components/navigation/tab-navigation-root.tsx (1)
TNavigationItem(31-40)
⏰ 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). (4)
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: Agent
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
apps/web/core/components/navigation/tab-navigation-root.tsx (1)
117-123: Destructuring matches updated hook API and is correctly wiredThe destructuring order and usage of
containerRefanditemRefsalign with the updateduseResponsiveTabLayoutreturn shape and are used correctly on the container and items.
anmolsinghbhatia
restored the
chore-navigation-and-sidebar-enhancements
branch
Description
This PR includes improvement for sidebar and tab navigation.
Type of Change
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.