regression: sidebar toggle button position #8186
Conversation
WalkthroughAdds conditional AppSidebarToggleButton rendering in several headers using theme state and route/navigation preferences, adjusts header padding and navigation spacing, removes rounded corners on a workspace wrapper, and wraps a project layout component with mobx-react's observer. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Header as Header component
participant Theme as useAppTheme (store)
participant Route as Router (useParams)
participant Prefs as useProjectNavigationPreferences
participant Toggle as AppSidebarToggleButton
Note over Theme,Route: Render-time checks
Header->>Theme: read sidebarCollapsed
Header->>Route: read params (projectId, workItem)
Header->>Prefs: read navigationMode
alt sidebarCollapsed AND (navigationMode == "accordion" OR no projectId/workItem)
Header->>Toggle: render AppSidebarToggleButton
Toggle-->>Header: visible
else
Header-->>Toggle: do not render
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,mts,cts}📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
Files:
⏰ 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 (1)
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 fixes the sidebar toggle button positioning when the sidebar is collapsed. The fix ensures proper visual alignment by moving the toggle button outside the TabNavigationRoot component and adjusting padding dynamically based on sidebar state.
Key Changes
- Removed left padding from tab navigation container to prevent misalignment when sidebar is collapsed
- Added conditional rendering logic to control when the sidebar toggle button appears (only in horizontal navigation mode or outside project/workItem contexts)
- Repositioned the toggle button to render alongside header components with consistent spacing
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tab-navigation-root.tsx | Removed pl-1.5 class to eliminate left padding from the tab navigation container |
| extended-app-header.tsx | Added conditional logic to show toggle button only in accordion mode or when not in project/workItem context |
| layout.tsx (ProjectLayout) | Added toggle button outside Header component with conditional padding on Header based on sidebar state |
| header.tsx (ProjectWorkItemDetailsHeader) | Added toggle button outside Header component with conditional padding on Header based on sidebar state |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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 (1)
apps/web/app/(all)/[workspaceSlug]/(projects)/browse/[workItem]/header.tsx (1)
5-51: Sidebar toggle + dynamic header padding logic looks consistent and preserves previous spacingUsing
sidebarCollapsedfrom the theme store both to renderAppSidebarToggleButtonand to conditionally apply"pl-1.5"viacnensures the tabs sit flush against the toggle when collapsed and keep their prior left offset when expanded. The flex wrapper (size-full flex-1) matches the layout header structure, so behavior should be consistent across routes.If more headers adopt this same pattern, it may be worth extracting a small shared “sidebar-aware header shell” component to avoid markup drift between screens over time. Also, continuing to rely on
useParams()here fits the project’s convention that the params prop is only used inpage.tsx/layout.tsxfiles. Based on learnings, this matches the intended routing pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/app/(all)/[workspaceSlug]/(projects)/browse/[workItem]/header.tsx(3 hunks)apps/web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/layout.tsx(3 hunks)apps/web/ce/components/common/extended-app-header.tsx(1 hunks)apps/web/core/components/navigation/tab-navigation-root.tsx(1 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/ce/components/common/extended-app-header.tsxapps/web/core/components/navigation/tab-navigation-root.tsxapps/web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/layout.tsxapps/web/app/(all)/[workspaceSlug]/(projects)/browse/[workItem]/header.tsx
🧠 Learnings (4)
📚 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/ce/components/common/extended-app-header.tsxapps/web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/layout.tsxapps/web/app/(all)/[workspaceSlug]/(projects)/browse/[workItem]/header.tsx
📚 Learning: 2025-10-09T20:42:31.843Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7922
File: apps/admin/app/(all)/(dashboard)/ai/form.tsx:19-19
Timestamp: 2025-10-09T20:42:31.843Z
Learning: In the makeplane/plane repository, React types are globally available through TypeScript configuration. Type annotations like React.FC, React.ReactNode, etc. can be used without explicitly importing the React namespace. The codebase uses the modern JSX transform, so React imports are not required for JSX or type references.
Applied to files:
apps/web/app/(all)/[workspaceSlug]/(projects)/browse/[workItem]/header.tsx
📚 Learning: 2025-05-14T13:16:23.323Z
Learnt from: vamsikrishnamathala
Repo: makeplane/plane PR: 7061
File: web/core/components/workspace-notifications/root.tsx:18-18
Timestamp: 2025-05-14T13:16:23.323Z
Learning: In the Plane project codebase, the path alias `@/plane-web` resolves to the `ce` directory, making imports like `@/plane-web/hooks/use-notification-preview` correctly resolve to files in `web/ce/hooks/`.
Applied to files:
apps/web/app/(all)/[workspaceSlug]/(projects)/browse/[workItem]/header.tsx
📚 Learning: 2025-10-09T20:43:07.762Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7922
File: apps/admin/core/components/new-user-popup.tsx:4-6
Timestamp: 2025-10-09T20:43:07.762Z
Learning: The `next-themes` library is React-compatible and can be used outside of Next.js applications. It's not Next.js-specific despite its name.
Applied to files:
apps/web/app/(all)/[workspaceSlug]/(projects)/browse/[workItem]/header.tsx
🧬 Code graph analysis (3)
apps/web/ce/components/common/extended-app-header.tsx (3)
apps/space/app/compat/next/navigation.ts (1)
useParams(34-36)apps/web/core/hooks/use-navigation-preferences.ts (1)
useProjectNavigationPreferences(107-155)apps/web/core/components/sidebar/sidebar-toggle-button.tsx (1)
AppSidebarToggleButton(6-21)
apps/web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/layout.tsx (2)
apps/web/core/components/sidebar/sidebar-toggle-button.tsx (1)
AppSidebarToggleButton(6-21)packages/ui/src/header/header.tsx (1)
Header(76-76)
apps/web/app/(all)/[workspaceSlug]/(projects)/browse/[workItem]/header.tsx (2)
apps/web/core/components/sidebar/sidebar-toggle-button.tsx (1)
AppSidebarToggleButton(6-21)packages/ui/src/header/header.tsx (1)
Header(76-76)
⏰ 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). (3)
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
apps/web/core/components/navigation/tab-navigation-root.tsx (1)
169-246: Padding removal from tab navigation container aligns with header‑controlled spacingDropping the internal
pl-1.5keepsTabNavigationRootlayout-neutral and lets callers (the various headers) own leading padding so the sidebar toggle and tabs align without double indentation. This is a safe, layout-only adjustment.apps/web/ce/components/common/extended-app-header.tsx (1)
3-26: DerivedshouldShowSidebarToggleButtoncleanly avoids duplicate togglesThe combination of navigation preferences and route params to compute
shouldShowSidebarToggleButton, then gating the button onsidebarCollapsed && shouldShowSidebarToggleButton, nicely keeps the toggle visible for accordion/global views while avoiding a second toggle on horizontal project/work-item screens that now render their own.Please double-check that the route param names (
projectId,workItem) match the React Router path definitions for the views where this header is used so the condition behaves as intended.apps/web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/layout.tsx (1)
3-52: Observer‑wrapped layout with sidebar toggle + header padding matches the new patternReading
sidebarCollapsedvia the theme hook and using it both to renderAppSidebarToggleButtonand to driveHeader’s conditional"pl-1.5"keeps the project detail header visually in sync with the work‑item header for collapsed vs expanded sidebar states. Moving the component into a namedProjectLayoutfunction and exportingobserver(ProjectLayout)also aligns with the convention of using theparamsprop (Route.ComponentProps) instead ofuseParams()specifically inlayout.tsxfiles. Based on learnings, this matches the intended app‑router usage.It would still be good to run through the affected project detail route to confirm the layout continues to be recognized correctly by the router and that MobX‑driven sidebar state updates re-render as expected.
* fix: sidebar toggle button position * chore: remove border radius
* fix: sidebar toggle button position * chore: remove border radius
* fix: sidebar toggle button position * chore: remove border radius
Description
This PR fixes the sidebar toggle button position when the sidebar is collapsed.
Type of Change
Summary by CodeRabbit
New Features
UI Improvements
✏️ Tip: You can customize this high-level summary in your review settings.