[WEB-4827] chore: sidebar content wrapper and code refactor#7840
[WEB-4827] chore: sidebar content wrapper and code refactor#7840anmolsinghbhatia wants to merge 1 commit intopreviewfrom
Conversation
anmolsinghbhatia
requested review from
prateekshourya29 and
vamsikrishnamathala
WalkthroughIntroduces a new SidebarContentWrapper component and exports it via the CE sidebar barrel. Updates core SidebarWrapper to use this wrapper and adjusts layout/positioning of WorkspaceEditionBadge and app-rail conditionals. Modifies WorkspaceEditionBadge to render HelpMenu/AppSidebarToggleButton conditionally. Simplifies dialog root utility by removing a useCallback. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant SW as SidebarWrapper
participant AR as useAppRail
participant SCW as SidebarContentWrapper
participant WEB as WorkspaceEditionBadge
U->>SW: Render sidebar
SW->>SCW: Wrap children
SW->>WEB: Render edition badge
SW->>AR: Query app-rail state (isEnabled, shouldRender)
Note over SW,AR: Conditional UI outside bottom area
rect rgba(200,230,255,0.25)
note right of SCW: New wrapper component for content scroll/layout
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the sidebar content wrapper and improves code organization. The main purpose is to extract reusable components and optimize React usage patterns.
- Converts a React.useCallback hook to a regular function in the dialog root component
- Extracts sidebar content wrapper logic into a dedicated reusable component
- Consolidates sidebar footer elements (help menu, toggle button) into the edition badge component
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/propel/src/dialog/root.tsx | Removes unnecessary useCallback wrapper for position class names function |
| apps/web/core/components/sidebar/sidebar-wrapper.tsx | Replaces inline content wrapper with extracted component and moves footer elements |
| apps/web/ce/components/workspace/edition-badge.tsx | Adds footer layout wrapper and integrates help menu and toggle button |
| apps/web/ce/components/sidebar/sidebar-content-wrapper.tsx | Creates new reusable sidebar content wrapper component |
| apps/web/ce/components/sidebar/index.ts | Exports the new sidebar content wrapper component |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| import packageJson from "package.json"; | ||
| import { useTranslation } from "@plane/i18n"; | ||
| import { Tooltip } from "@plane/propel/tooltip"; | ||
| import { Button } from "@plane/ui"; |
There was a problem hiding this comment.
[nitpick] The packageJson import should be moved after the React-related imports to follow conventional import ordering (external dependencies, then internal dependencies).
| import packageJson from "package.json"; | |
| import { useTranslation } from "@plane/i18n"; | |
| import { Tooltip } from "@plane/propel/tooltip"; | |
| import { Button } from "@plane/ui"; | |
| import { useTranslation } from "@plane/i18n"; | |
| import { Tooltip } from "@plane/propel/tooltip"; | |
| import { Button } from "@plane/ui"; | |
| import packageJson from "package.json"; |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on October 20
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| }; | ||
|
|
||
| export const SidebarContentWrapper = ({ children }: TSidebarContentWrapperProps) => ( | ||
| <div className="flex flex-col gap-3 overflow-x-hidden scrollbar-sm h-full w-full overflow-y-auto vertical-scrollbar px-3 pt-3"> |
There was a problem hiding this comment.
Bug: Sidebar Content Padding Issue
Missing pb-0.5 class in the SidebarContentWrapper. The original div in sidebar-wrapper.tsx had pb-0.5 for bottom padding, but the new SidebarContentWrapper component omits this class, which could affect the bottom spacing of the scrollable content.
| {!shouldRenderAppRail && <HelpMenu />} | ||
| {!isAppRailEnabled && <AppSidebarToggleButton />} | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Bug: Modal Rendering Inside Positioned Container
The PaidPlanUpgradeModal component is now rendered inside a positioned div container with specific styling (border, background, padding, height). Previously, it was rendered at the top level in a React fragment. This change could cause modal positioning or z-index issues since modals should typically be rendered at the DOM tree's top level, not inside positioned containers, even when using portals.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/propel/src/dialog/root.tsx (1)
76-95:useMemohere is unnecessary; compute classNames inline.The class string is cheap to build; removing
useMemoreduces hooks noise without measurable perf loss.Apply this diff:
({ className, width = EDialogWidth.XXL, children, position = "center", ...props }, ref) => { - const positionClassNames = React.useMemo(() => getPositionClassNames(position), [position]); return ( <DialogPortal> <DialogOverlay /> <BaseDialog.Popup ref={ref} data-slot="dialog-content" - className={cn(BASE_CLASSNAME, positionClassNames, width, className)} + className={cn(BASE_CLASSNAME, getPositionClassNames(position), width, className)} role="dialog" aria-modal="true" {...props} > {children} </BaseDialog.Popup> </DialogPortal> ); }apps/web/ce/components/workspace/edition-badge.tsx (1)
4-4: Avoid importing full package.json in client bundles; prefer an env-exposed version.Importing package.json can ship extra metadata to the client bundle. Use a public env var for version, with a fallback for dev.
Apply this diff to add a safe version constant near imports:
import packageJson from "package.json"; +const appVersion = process.env.NEXT_PUBLIC_APP_VERSION ?? packageJson.version;And update tooltip content to use
appVersion:// at Line 33 <Tooltip tooltipContent={`Version: v${appVersion}`} isMobile={isMobile}>apps/web/ce/components/sidebar/sidebar-content-wrapper.tsx (1)
7-10: Spacing nit: potential double spacing with parent’s gap.Parent adds a
gap-3between sections and this wrapper addspt-3, which may stack. Consider droppingpt-3here or the parent’s bottom gap for tighter control.apps/web/core/components/sidebar/sidebar-wrapper.tsx (1)
44-59: Watch for stacked vertical spacing between header and content.Header section has
gap-3; the content wrapper also haspt-3. If spacing looks too large, remove one of them.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/web/ce/components/sidebar/index.ts(1 hunks)apps/web/ce/components/sidebar/sidebar-content-wrapper.tsx(1 hunks)apps/web/ce/components/workspace/edition-badge.tsx(3 hunks)apps/web/core/components/sidebar/sidebar-wrapper.tsx(3 hunks)packages/propel/src/dialog/root.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/web/ce/components/workspace/edition-badge.tsx (4)
apps/web/core/hooks/use-app-rail.tsx (1)
useAppRail(4-10)packages/hooks/src/use-platform-os.tsx (1)
usePlatformOS(3-34)apps/web/core/components/workspace/sidebar/help-menu.tsx (1)
HelpMenu(26-134)apps/web/core/components/sidebar/sidebar-toggle-button.tsx (1)
AppSidebarToggleButton(8-23)
apps/web/core/components/sidebar/sidebar-wrapper.tsx (2)
apps/web/ce/components/sidebar/sidebar-content-wrapper.tsx (1)
SidebarContentWrapper(7-11)apps/web/ce/components/workspace/edition-badge.tsx (1)
WorkspaceEditionBadge(16-51)
packages/propel/src/dialog/root.tsx (1)
packages/propel/src/utils/classname.tsx (1)
cn(4-4)
⏰ 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 (7)
packages/propel/src/dialog/root.tsx (1)
44-48: Good call makinggetPositionClassNamesa pure util.The simplified helper is clear and keeps class composition straightforward.
apps/web/ce/components/workspace/edition-badge.tsx (2)
21-23: ConfirmWorkspaceEditionBadgealways renders withinAppRailProvider.
useAppRailthrows if the provider is missing. Verify this component isn't used outside the provider after the layout changes.
46-48: Conditional rendering looks correct.
HelpMenuwhen the rail isn't rendered and the toggle when the rail is disabled aligns with the new layout logic.apps/web/ce/components/sidebar/index.ts (1)
3-3: Barrel export addition looks good.Re-exporting
SidebarContentWrapperfrom the CE sidebar index is appropriate.apps/web/core/components/sidebar/sidebar-wrapper.tsx (3)
13-13: Correct import of the shared wrapper.Pulling
SidebarContentWrapperfrom the CE barrel keeps imports consistent.
43-43: Containerrelativeis fine.Sets a positioning context for any absolutely positioned elements inside.
61-61: MovingWorkspaceEditionBadgeto the bottom is a clean separation.This aligns with the new wrapper and keeps controls consistent.
| export const SidebarContentWrapper = ({ children }: TSidebarContentWrapperProps) => ( | ||
| <div className="flex flex-col gap-3 overflow-x-hidden scrollbar-sm h-full w-full overflow-y-auto vertical-scrollbar px-3 pt-3"> | ||
| {children} | ||
| </div> |
There was a problem hiding this comment.
Fix flex/height: h-full here will force overflow and hide the bottom bar.
When used between a header and a footer in a column flex container, this middle section should flex, not be h-full. Use flex-1 min-h-0 to allow internal scrolling and keep the footer visible.
Apply this diff:
-export const SidebarContentWrapper = ({ children }: TSidebarContentWrapperProps) => (
- <div className="flex flex-col gap-3 overflow-x-hidden scrollbar-sm h-full w-full overflow-y-auto vertical-scrollbar px-3 pt-3">
+export const SidebarContentWrapper = ({ children }: TSidebarContentWrapperProps) => (
+ <div className="flex flex-col gap-3 overflow-x-hidden scrollbar-sm flex-1 min-h-0 w-full overflow-y-auto vertical-scrollbar px-3 pt-3">
{children}
</div>
);📝 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 SidebarContentWrapper = ({ children }: TSidebarContentWrapperProps) => ( | |
| <div className="flex flex-col gap-3 overflow-x-hidden scrollbar-sm h-full w-full overflow-y-auto vertical-scrollbar px-3 pt-3"> | |
| {children} | |
| </div> | |
| export const SidebarContentWrapper = ({ children }: TSidebarContentWrapperProps) => ( | |
| <div className="flex flex-col gap-3 overflow-x-hidden scrollbar-sm flex-1 min-h-0 w-full overflow-y-auto vertical-scrollbar px-3 pt-3"> | |
| {children} | |
| </div> | |
| ); |
🤖 Prompt for AI Agents
In apps/web/ce/components/sidebar/sidebar-content-wrapper.tsx around lines 7 to
10, the wrapper uses h-full which forces full height and can push the footer out
of view; change the container class to use flex-1 min-h-0 instead of h-full so
the middle section flexes between header and footer and internal scrolling works
(keep the existing overflow-y-auto/overflow-x-hidden and other spacing classes).
| <div className="flex flex-col gap-3 overflow-x-hidden scrollbar-sm h-full w-full overflow-y-auto vertical-scrollbar px-3 pt-3 pb-0.5"> | ||
| {children} | ||
| </div> | ||
| <SidebarContentWrapper>{children}</SidebarContentWrapper> |
There was a problem hiding this comment.
🧩 Analysis chain
Ensure the middle section flexes; otherwise the bottom badge may be pushed off-screen.
After switching to SidebarContentWrapper, make sure it uses flex-1 min-h-0 (not h-full). See suggested change in the wrapper component.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show the referenced file around the snippet
echo "=== apps/web/core/components/sidebar/sidebar-wrapper.tsx ==="
sed -n '1,240p' apps/web/core/components/sidebar/sidebar-wrapper.tsx || true
echo
# 2) Search the repo for SidebarContentWrapper definition/usage
echo "=== rg matches for SidebarContentWrapper ==="
rg -n "SidebarContentWrapper" || true
echo
# 3) If a definition file is found, print its contents (attempt common patterns)
echo "=== Possible wrapper component files (searching for 'Sidebar' files) ==="
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!**/build/**' "Sidebar" || true
echo
# 4) Search for components in the same directory
echo "=== Files in apps/web/core/components/sidebar ==="
ls -la apps/web/core/components/sidebar || true
echo
# 5) If a file named sidebar-content-wrapper.* exists, print it
if [ -f apps/web/core/components/sidebar/sidebar-content-wrapper.tsx ]; then
echo "=== apps/web/core/components/sidebar/sidebar-content-wrapper.tsx ==="
sed -n '1,240p' apps/web/core/components/sidebar/sidebar-content-wrapper.tsx || true
fi
# 6) Grep for patterns of flex-1 or min-h-0 in the sidebar components
echo "=== Search for 'flex-1' and 'min-h-0' in sidebar dir ==="
rg -n "flex-1|min-h-0|h-full" apps/web/core/components/sidebar || trueLength of output: 4850
---
🏁 Script executed:
```shell
#!/bin/bash
set -euo pipefail
echo "=== files in apps/web/ce/components/sidebar ==="
ls -la apps/web/ce/components/sidebar || true
echo
echo "=== apps/web/ce/components/sidebar/sidebar-content-wrapper.tsx ==="
sed -n '1,240p' apps/web/ce/components/sidebar/sidebar-content-wrapper.tsx || true
echo
echo "=== Search for className usage in that file ==="
rg -n "className" apps/web/ce/components/sidebar/sidebar-content-wrapper.tsx || true
echo
echo "=== Search for flex-1|min-h-0|h-full in that directory ==="
rg -n "flex-1|min-h-0|h-full" apps/web/ce/components/sidebar || true
echo
echo "=== Search for exports in apps/web/ce/components/sidebar ==="
rg -n "export .*SidebarContentWrapper|export \\{.*SidebarContentWrapper" apps/web/ce/components/sidebar || true
echo
echo "=== Search for 'plane-web/components/sidebar' alias usages in repo ==="
rg -n "@/plane-web/components/sidebar" || true
Length of output: 1815
Make SidebarContentWrapper use flex-1 + min-h-0 (not h-full)
apps/web/ce/components/sidebar/sidebar-content-wrapper.tsx — replace the root div's "h-full w-full" with "flex-1 min-h-0" (e.g. "flex-1 min-h-0 flex flex-col gap-3 overflow-x-hidden scrollbar-sm overflow-y-auto vertical-scrollbar px-3 pt-3") so the middle section can shrink and the WorkspaceEditionBadge stays visible.
Description
This PR includes refactoring of the sidebar content wrapper and sidebar badge.
Type of Change
Summary by CodeRabbit
New Features
Refactor
Chores