Refactor layout to use shadcn sidebar primitives#56
Open
mrabbani wants to merge 1 commit intofeature/show-icon-minimize-sidebarfrom
Open
Refactor layout to use shadcn sidebar primitives#56mrabbani wants to merge 1 commit intofeature/show-icon-minimize-sidebarfrom
mrabbani wants to merge 1 commit intofeature/show-icon-minimize-sidebarfrom
Conversation
Refactor the Layout system to use shadcn Sidebar components instead of custom responsive sidebar, mobile drawer, and flyout implementations. This removes ~1500 lines of custom code and replaces them with standard shadcn primitives while preserving the data-driven LayoutMenu API. Key changes: - Add shadcn sidebar.tsx and sheet.tsx primitives - Layout now wraps SidebarProvider, LayoutSidebar wraps Sidebar - LayoutMenu uses SidebarMenu/SidebarMenuItem/SidebarMenuButton - Add useSidebarOptional for standalone LayoutMenu usage - Add useIsMobile hook with synchronous init (prevents render flash) - Suppress CSS transitions on mount via data-mounted attribute - Use theme-compatible border-sidebar-border colors - Move layout components from ui/ to wordpress/ directory - Rewrite stories with 11 focused examples including sidebar footer Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR refactors the layout system by moving Layout and LayoutMenu components from generic UI ( Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant SidebarProvider
participant Layout
participant LayoutMenu
participant SidebarContext
App->>SidebarProvider: Wrap Layout
SidebarProvider->>SidebarContext: Initialize state (open, isMobile, toggle)
SidebarProvider->>App: Provide context
App->>Layout: Render with namespace prop
Layout->>LayoutMenu: Pass items/groups and activeItemId
LayoutMenu->>LayoutMenu: Filter items via search
LayoutMenu->>SidebarContext: Consume sidebar state
LayoutMenu->>LayoutMenu: Render MenuItemRenderer recursively
LayoutMenu-->>App: Emit onItemClick events
Note over SidebarContext: Keyboard shortcut (Ctrl+B)<br/>toggles sidebar state
SidebarContext-->>Layout: setSidebarOpen via context
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 5
🧹 Nitpick comments (7)
src/components/ui/index.ts (1)
231-256: Consider adding a comment clarifying why WordPress modules are re-exported from the UI barrel.The
ui/index.tsbarrel re-exports Layout and LayoutMenu from../wordpress/. This is fine for backward compatibility, but the file header states "All components are pure React - no WordPress dependencies" (line 2). The WordPress layout modules may have WordPress hook dependencies (e.g.,doAction). A brief comment noting this is a re-export for convenience would avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/index.ts` around lines 231 - 256, Add a short clarifying comment above the export blocks for Layout and LayoutMenu stating these are re-exports of WordPress-specific modules for backward compatibility/convenience and may bring WordPress hook dependencies (e.g., doAction) despite the file header claiming "pure React"; reference the exported symbols Layout, LayoutHeader, LayoutBody, LayoutMain, LayoutSidebar, LayoutFooter and LayoutMenu/LayoutMenuSearch so future maintainers know why these modules live in this UI barrel and to watch for WP hooks.src/components/wordpress/layout-menu.tsx (3)
351-503:MenuItemRendererandSubMenuItemRenderershare significant duplicated logic.Both renderers duplicate:
containsActivememo, auto-expand effect,handleClickcallback,renderItemearly return, and active-state styling. Consider extracting a shared hook (e.g.,useMenuItemState) to reduce duplication.Example shared hook
function useMenuItemState( item: LayoutMenuItemData, activeItemId: string | null | undefined, onItemClick?: (item: LayoutMenuItemData) => void, ) { const hasChildren = item.children && item.children.length > 0; const isActive = activeItemId != null && item.id === activeItemId; const containsActive = useMemo( () => hasActiveDescendant(item, activeItemId), [item, activeItemId] ); const [open, setOpen] = useState(containsActive); useEffect(() => { if (containsActive) setOpen(true); }, [containsActive]); const handleClick = useCallback(() => { if (hasChildren) { setOpen((o) => !o); } else { item.onClick?.(); } onItemClick?.(item); }, [hasChildren, item, onItemClick]); return { hasChildren, isActive, containsActive, open, setOpen, handleClick }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/wordpress/layout-menu.tsx` around lines 351 - 503, MenuItemRenderer and SubMenuItemRenderer duplicate state/logic (hasChildren, isActive, containsActive memo, open state with effect, and handleClick); extract these into a shared hook (e.g., useMenuItemState) that accepts (item, activeItemId, onItemClick) and returns { hasChildren, isActive, containsActive, open, setOpen, handleClick }, then replace the duplicated declarations in MenuItemRenderer and SubMenuItemRenderer with calls to useMenuItemState and use the returned values (ensure containsActive-driven auto-expand effect and handleClick behavior remain identical and update any references to open/setOpen/isActive/handleClick accordingly).
134-172: Minor:forwardRefis no longer required in React 19.Since
refis a regular prop in React 19,LayoutMenuSearchandLayoutMenucould acceptrefdirectly as a prop. Not urgent —forwardRefstill works — but worth noting for a future modernization pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/wordpress/layout-menu.tsx` around lines 134 - 172, The component uses React.forwardRef (LayoutMenuSearch) even though React 19 treats ref as a normal prop; to modernize, remove forwardRef and convert LayoutMenuSearch (and similarly LayoutMenu if present) to accept a ref prop directly by changing the component signature to accept ref in its props and stopping use of forwardRef, ensuring you keep the same prop names (value, onChange, placeholder, className, inputClassName) and still pass ref into the root div and set LayoutMenuSearch.displayName = "LayoutMenuSearch"; update any consumers that passed refs via forwardRef if necessary.
88-105: Search filtering:matchesSearchalready covers descendants, makingchildMatchat line 101 effectively unreachable.
matchesSearchrecursively checks children (line 84), so wheneverfilterMenuItemsreturns non-emptychildMatch,selfMatchwould already betrue, and line 100 returns first. ThechildMatchbranch (line 101) appears to be dead code.This doesn't cause incorrect behavior — the filter results are correct either way — but it does perform unnecessary recursive work via
filterMenuItemson children even whenselfMatchwill short-circuit to returning the full item.A minor optimization would be to check
selfMatchfirst, then only computechildMatchif needed:Lazy child filtering
return items .map((item) => { - const childMatch = item.children?.length - ? filterMenuItems(item.children, query) - : []; const selfMatch = matchesSearch(item, query); if (selfMatch) return item; + const childMatch = item.children?.length + ? filterMenuItems(item.children, query) + : []; if (childMatch.length > 0) return { ...item, children: childMatch }; return null; })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/wordpress/layout-menu.tsx` around lines 88 - 105, In filterMenuItems, avoid computing childMatch for every item because matchesSearch already checks descendants; first compute selfMatch via matchesSearch(item, query) and if true return item immediately, and only then compute childMatch = item.children?.length ? filterMenuItems(item.children, query) : [] to build a pruned copy when selfMatch is false; update references to use the trimmed/lowercased q where appropriate so recursion and matching use the same normalized query.src/components/ui/sidebar.tsx (1)
491-510: Redundant hover styles in thedefaultvariant.The base class string of
sidebarMenuButtonVariantsalready includeshover:bg-sidebar-accent hover:text-sidebar-accent-foreground, and thedefaultvariant repeats exactly the same hover classes. The duplication has no runtime impact but adds unnecessary noise.Remove duplicate hover classes from the default variant
variant: { - default: "hover:bg-sidebar-accent hover:text-sidebar-accent-foreground", + default: "", outline: "bg-background hover:bg-sidebar-accent hover:text-sidebar-accent-foreground shadow-[0_0_0_1px_hsl(var(--sidebar-border))] hover:shadow-[0_0_0_1px_hsl(var(--sidebar-accent))]", },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/sidebar.tsx` around lines 491 - 510, In sidebarMenuButtonVariants remove the duplicated hover classes from the default variant: the base class string already contains "hover:bg-sidebar-accent hover:text-sidebar-accent-foreground", so edit the variants.variant.default entry to drop those two classes (leave any other classes in that variant untouched) to eliminate the redundant repetition.src/hooks/use-mobile.ts (1)
14-17: Consider usingmql.matchesinstead of re-readingwindow.innerWidth.The
onChangehandler re-readswindow.innerWidthrather than using theMediaQueryListevent'smatchesproperty. While the values should be equivalent in most cases,mql.matchesis the canonical source of truth for achangelistener and avoids potential subtle discrepancies (e.g., scrollbar-inclusive vs content width).Proposed fix
const onChange = () => { - setIsMobile(window.innerWidth < MOBILE_BREAKPOINT) + setIsMobile(mql.matches) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/use-mobile.ts` around lines 14 - 17, The onChange handler in use-mobile.ts should use the MediaQueryList's matches value instead of re-reading window.innerWidth; update the onChange function (and its listener attachment for mql) to accept the MediaQueryListEvent (or read mql.matches if no event param) and call setIsMobile(event.matches || mql.matches), referencing mql, onChange, setIsMobile and MOBILE_BREAKPOINT to ensure the media-query's canonical matches value is used and to provide a safe fallback for environments without event support.src/components/wordpress/layout.tsx (1)
63-65: Shared namespace collision risk when multiple Layout instances coexist.The default
namespace="plugin_ui"(line 64) creates a collision vulnerability. Lines 113–116 register and remove WordPress action handlers by namespace group; multiple Layout instances will overwrite or unregister each other's handlers becauseremoveActionclears all callbacks in that namespace group.Currently this affects only Storybook stories (10 instances all using the default), but the component is a reusable library primitive. Recommend making
namespacea required prop to enforce explicit namespace assignment per instance:Suggested refactor
export interface LayoutProps extends HTMLAttributes<HTMLDivElement> { - namespace?: string; + namespace: string; } export const Layout = forwardRef<HTMLDivElement, LayoutProps>( ( { className, defaultSidebarOpen = false, - namespace = "plugin_ui", + namespace, children,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/wordpress/layout.tsx` around lines 63 - 65, The Layout component currently defaults namespace="plugin_ui", causing shared namespace collisions when multiple instances add/remove WordPress actions; remove the default and make the namespace prop required on the Layout component (no fallback) so each consumer must pass a unique namespace string, update all references inside Layout that call addAction/removeAction to use the passed-in namespace prop (the same identifier "namespace" in the Layout signature), and update any callers (e.g., Storybook stories) to supply unique namespace values for each instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/ui/sidebar.tsx`:
- Around line 193-217: The mobile branch of Sidebar spreads component props
(props) onto <Sheet>, which can forward invalid DOM-only props to the
Dialog-root wrapper; fix by destructuring the incoming props in Sidebar to
separate DOM-specific div props from props intended for Sheet (e.g., const {
children, style, className, ...restDivProps } = props) and only forward the
intended subset to <Sheet> (or explicitly pass allowed Sheet props like
open/onOpenChange and className), while applying the remaining div props to the
desktop container (or change the Sidebar API to accept separate
sheetProps/divProps). Update usages of openMobile, setOpenMobile and children to
use the new prop split so mobile rendering no longer spreads arbitrary div props
onto <Sheet>.
In `@src/components/wordpress/Layout.stories.tsx`:
- Around line 180-184: Escape the literal double-quote characters inside the JSX
text to satisfy the static analysis: update the text within the LayoutMain block
that references side="right" (and any similar inline examples like
LayoutSidebar) to use HTML entity " (or use a curly-brace string
expression) instead of raw " characters so the JSX text no longer contains
unescaped quotes.
- Around line 237-266: The Story object ActiveItemTracking currently calls
useState inside its render function (named render) which violates the React
hooks lint rule; extract the stateful JSX into a separate React component (e.g.,
ActiveItemTrackingComponent) that declares the hook (useState for activeId and
setActiveId) and returns the Layout tree, then change ActiveItemTracking.render
to return <ActiveItemTrackingComponent /> (or reference the new component) so
the hook lives in a properly capitalized component; update any inline handlers
(onItemClick) to use the component's setActiveId.
- Around line 440-444: The JSX text contains raw double-quote characters causing
lint/CI failures; update the paragraph in Layout.stories.tsx so the quotes are
escaped (e.g., change doAction("myapp_layout_toggle") to
doAction("myapp_layout_toggle") and change <code>namespace</code> if
it contains literals with quotes similarly) or alternatively render the quoted
strings using JS string nodes (e.g., {'"myapp_layout_toggle"'}); modify the <p>
content where the text and <code>doAction("myapp_layout_toggle")</code> appears
to use " or {'"'} for the quotes so the JSX no longer contains unescaped "
characters.
In `@src/components/wordpress/layout.tsx`:
- Around line 247-252: LayoutSidebar's ref typing is wrong and uses React.Ref
without importing React and mismatches HTMLElement vs HTMLDivElement; change the
forwardRef generic from HTMLElement to HTMLDivElement and import the type Ref
from "react" (e.g., import type { forwardRef, Ref } from "react") then cast the
ref passed into Sidebar as Ref<HTMLDivElement> (or better, keep ref typed as the
forwarded HTMLDivElement) so the forwardRef generic, the ref cast in the
LayoutSidebar component, and the Sidebar ref type all align.
---
Nitpick comments:
In `@src/components/ui/index.ts`:
- Around line 231-256: Add a short clarifying comment above the export blocks
for Layout and LayoutMenu stating these are re-exports of WordPress-specific
modules for backward compatibility/convenience and may bring WordPress hook
dependencies (e.g., doAction) despite the file header claiming "pure React";
reference the exported symbols Layout, LayoutHeader, LayoutBody, LayoutMain,
LayoutSidebar, LayoutFooter and LayoutMenu/LayoutMenuSearch so future
maintainers know why these modules live in this UI barrel and to watch for WP
hooks.
In `@src/components/ui/sidebar.tsx`:
- Around line 491-510: In sidebarMenuButtonVariants remove the duplicated hover
classes from the default variant: the base class string already contains
"hover:bg-sidebar-accent hover:text-sidebar-accent-foreground", so edit the
variants.variant.default entry to drop those two classes (leave any other
classes in that variant untouched) to eliminate the redundant repetition.
In `@src/components/wordpress/layout-menu.tsx`:
- Around line 351-503: MenuItemRenderer and SubMenuItemRenderer duplicate
state/logic (hasChildren, isActive, containsActive memo, open state with effect,
and handleClick); extract these into a shared hook (e.g., useMenuItemState) that
accepts (item, activeItemId, onItemClick) and returns { hasChildren, isActive,
containsActive, open, setOpen, handleClick }, then replace the duplicated
declarations in MenuItemRenderer and SubMenuItemRenderer with calls to
useMenuItemState and use the returned values (ensure containsActive-driven
auto-expand effect and handleClick behavior remain identical and update any
references to open/setOpen/isActive/handleClick accordingly).
- Around line 134-172: The component uses React.forwardRef (LayoutMenuSearch)
even though React 19 treats ref as a normal prop; to modernize, remove
forwardRef and convert LayoutMenuSearch (and similarly LayoutMenu if present) to
accept a ref prop directly by changing the component signature to accept ref in
its props and stopping use of forwardRef, ensuring you keep the same prop names
(value, onChange, placeholder, className, inputClassName) and still pass ref
into the root div and set LayoutMenuSearch.displayName = "LayoutMenuSearch";
update any consumers that passed refs via forwardRef if necessary.
- Around line 88-105: In filterMenuItems, avoid computing childMatch for every
item because matchesSearch already checks descendants; first compute selfMatch
via matchesSearch(item, query) and if true return item immediately, and only
then compute childMatch = item.children?.length ? filterMenuItems(item.children,
query) : [] to build a pruned copy when selfMatch is false; update references to
use the trimmed/lowercased q where appropriate so recursion and matching use the
same normalized query.
In `@src/components/wordpress/layout.tsx`:
- Around line 63-65: The Layout component currently defaults
namespace="plugin_ui", causing shared namespace collisions when multiple
instances add/remove WordPress actions; remove the default and make the
namespace prop required on the Layout component (no fallback) so each consumer
must pass a unique namespace string, update all references inside Layout that
call addAction/removeAction to use the passed-in namespace prop (the same
identifier "namespace" in the Layout signature), and update any callers (e.g.,
Storybook stories) to supply unique namespace values for each instance.
In `@src/hooks/use-mobile.ts`:
- Around line 14-17: The onChange handler in use-mobile.ts should use the
MediaQueryList's matches value instead of re-reading window.innerWidth; update
the onChange function (and its listener attachment for mql) to accept the
MediaQueryListEvent (or read mql.matches if no event param) and call
setIsMobile(event.matches || mql.matches), referencing mql, onChange,
setIsMobile and MOBILE_BREAKPOINT to ensure the media-query's canonical matches
value is used and to provide a safe fallback for environments without event
support.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/components/settings/settings-sidebar.tsxsrc/components/ui/Layout.stories.tsxsrc/components/ui/index.tssrc/components/ui/layout-menu.tsxsrc/components/ui/layout.tsxsrc/components/ui/sheet.tsxsrc/components/ui/sidebar.tsxsrc/components/wordpress/Layout.stories.tsxsrc/components/wordpress/layout-menu.tsxsrc/components/wordpress/layout.tsxsrc/hooks/use-mobile.tssrc/index.ts
💤 Files with no reviewable changes (3)
- src/components/ui/layout.tsx
- src/components/ui/layout-menu.tsx
- src/components/ui/Layout.stories.tsx
Comment on lines
+193
to
+217
| if (isMobile) { | ||
| return ( | ||
| <Sheet open={openMobile} onOpenChange={setOpenMobile} {...props}> | ||
| <SheetContent | ||
| dir={dir} | ||
| data-sidebar="sidebar" | ||
| data-slot="sidebar" | ||
| data-mobile="true" | ||
| className="bg-sidebar text-sidebar-foreground w-(--sidebar-width) p-0 [&>button]:hidden" | ||
| style={ | ||
| { | ||
| "--sidebar-width": SIDEBAR_WIDTH_MOBILE, | ||
| } as React.CSSProperties | ||
| } | ||
| side={side} | ||
| > | ||
| <SheetHeader className="sr-only"> | ||
| <SheetTitle>Sidebar</SheetTitle> | ||
| <SheetDescription>Displays the mobile sidebar.</SheetDescription> | ||
| </SheetHeader> | ||
| <div className="flex h-full w-full flex-col">{children}</div> | ||
| </SheetContent> | ||
| </Sheet> | ||
| ) | ||
| } |
There was a problem hiding this comment.
Spreading ...props (div-typed) onto Sheet (Dialog.Root) may forward invalid DOM props.
Sidebar accepts React.ComponentProps<"div"> via ...props, but in the mobile branch, these are spread onto <Sheet> which wraps a Dialog.Root (not a DOM element). If consumers pass DOM-specific props (e.g., style, role, aria-*), they could be dropped or cause warnings.
Consider destructuring and forwarding only relevant props to Sheet, or document that extra div props are only applied in the desktop rendering path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/sidebar.tsx` around lines 193 - 217, The mobile branch of
Sidebar spreads component props (props) onto <Sheet>, which can forward invalid
DOM-only props to the Dialog-root wrapper; fix by destructuring the incoming
props in Sidebar to separate DOM-specific div props from props intended for
Sheet (e.g., const { children, style, className, ...restDivProps } = props) and
only forward the intended subset to <Sheet> (or explicitly pass allowed Sheet
props like open/onOpenChange and className), while applying the remaining div
props to the desktop container (or change the Sidebar API to accept separate
sheetProps/divProps). Update usages of openMobile, setOpenMobile and children to
use the new prop split so mobile rendering no longer spreads arbitrary div props
onto <Sheet>.
Comment on lines
+180
to
+184
| <LayoutMain> | ||
| <p className="text-muted-foreground"> | ||
| Pass <code>side="right"</code> to <code>LayoutSidebar</code> to place it on the right. | ||
| </p> | ||
| </LayoutMain> |
There was a problem hiding this comment.
CI failure: Escape literal quote characters in JSX text.
The static analysis flagged unescaped " characters in JSX text content on line 182. Use " or curly-brace expressions.
Proposed fix
<p className="text-muted-foreground">
- Pass <code>side="right"</code> to <code>LayoutSidebar</code> to place it on the right.
+ Pass <code>side="right"</code> to <code>LayoutSidebar</code> to place it on the right.
</p>📝 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.
Suggested change
| <LayoutMain> | |
| <p className="text-muted-foreground"> | |
| Pass <code>side="right"</code> to <code>LayoutSidebar</code> to place it on the right. | |
| </p> | |
| </LayoutMain> | |
| <LayoutMain> | |
| <p className="text-muted-foreground"> | |
| Pass <code>side="right"</code> to <code>LayoutSidebar</code> to place it on the right. | |
| </p> | |
| </LayoutMain> |
🧰 Tools
🪛 GitHub Check: build-and-test
[failure] 182-182:
" can be escaped with ", “, ", ”
[failure] 182-182:
" can be escaped with ", “, ", ”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/wordpress/Layout.stories.tsx` around lines 180 - 184, Escape
the literal double-quote characters inside the JSX text to satisfy the static
analysis: update the text within the LayoutMain block that references
side="right" (and any similar inline examples like LayoutSidebar) to use HTML
entity " (or use a curly-brace string expression) instead of raw "
characters so the JSX text no longer contains unescaped quotes.
Comment on lines
+237
to
+266
| export const ActiveItemTracking: Story = { | ||
| render: () => { | ||
| const [activeId, setActiveId] = useState("dashboard"); | ||
| return ( | ||
| <Layout defaultSidebarOpen className="bg-background"> | ||
| <LayoutHeader> | ||
| <span className="font-semibold">Active: {activeId}</span> | ||
| </LayoutHeader> | ||
| <LayoutBody> | ||
| <LayoutSidebar> | ||
| <LayoutMenu | ||
| items={sampleItems} | ||
| activeItemId={activeId} | ||
| searchable | ||
| onItemClick={(item) => { | ||
| if (!item.children?.length) setActiveId(item.id); | ||
| }} | ||
| /> | ||
| </LayoutSidebar> | ||
| <LayoutMain> | ||
| <p className="text-muted-foreground"> | ||
| Click a leaf menu item to change the active state. The current active item | ||
| is <strong>{activeId}</strong>. | ||
| </p> | ||
| </LayoutMain> | ||
| </LayoutBody> | ||
| </Layout> | ||
| ); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Fix: Extract the stateful render function into a named component to satisfy React hooks lint rule.
The CI build fails because useState is called inside a function named render which is neither a React component (no uppercase name) nor a custom hook. Extract the body into a proper component.
Proposed fix
+function ActiveItemTrackingStory() {
+ const [activeId, setActiveId] = useState("dashboard");
+ return (
+ <Layout defaultSidebarOpen className="bg-background">
+ <LayoutHeader>
+ <span className="font-semibold">Active: {activeId}</span>
+ </LayoutHeader>
+ <LayoutBody>
+ <LayoutSidebar>
+ <LayoutMenu
+ items={sampleItems}
+ activeItemId={activeId}
+ searchable
+ onItemClick={(item) => {
+ if (!item.children?.length) setActiveId(item.id);
+ }}
+ />
+ </LayoutSidebar>
+ <LayoutMain>
+ <p className="text-muted-foreground">
+ Click a leaf menu item to change the active state. The current active item
+ is <strong>{activeId}</strong>.
+ </p>
+ </LayoutMain>
+ </LayoutBody>
+ </Layout>
+ );
+}
+
/** Active item tracking — click items to navigate. */
export const ActiveItemTracking: Story = {
- render: () => {
- const [activeId, setActiveId] = useState("dashboard");
- return (
- <Layout defaultSidebarOpen className="bg-background">
- ...
- </Layout>
- );
- },
+ render: () => <ActiveItemTrackingStory />,
};📝 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.
Suggested change
| export const ActiveItemTracking: Story = { | |
| render: () => { | |
| const [activeId, setActiveId] = useState("dashboard"); | |
| return ( | |
| <Layout defaultSidebarOpen className="bg-background"> | |
| <LayoutHeader> | |
| <span className="font-semibold">Active: {activeId}</span> | |
| </LayoutHeader> | |
| <LayoutBody> | |
| <LayoutSidebar> | |
| <LayoutMenu | |
| items={sampleItems} | |
| activeItemId={activeId} | |
| searchable | |
| onItemClick={(item) => { | |
| if (!item.children?.length) setActiveId(item.id); | |
| }} | |
| /> | |
| </LayoutSidebar> | |
| <LayoutMain> | |
| <p className="text-muted-foreground"> | |
| Click a leaf menu item to change the active state. The current active item | |
| is <strong>{activeId}</strong>. | |
| </p> | |
| </LayoutMain> | |
| </LayoutBody> | |
| </Layout> | |
| ); | |
| }, | |
| }; | |
| function ActiveItemTrackingStory() { | |
| const [activeId, setActiveId] = useState("dashboard"); | |
| return ( | |
| <Layout defaultSidebarOpen className="bg-background"> | |
| <LayoutHeader> | |
| <span className="font-semibold">Active: {activeId}</span> | |
| </LayoutHeader> | |
| <LayoutBody> | |
| <LayoutSidebar> | |
| <LayoutMenu | |
| items={sampleItems} | |
| activeItemId={activeId} | |
| searchable | |
| onItemClick={(item) => { | |
| if (!item.children?.length) setActiveId(item.id); | |
| }} | |
| /> | |
| </LayoutSidebar> | |
| <LayoutMain> | |
| <p className="text-muted-foreground"> | |
| Click a leaf menu item to change the active state. The current active item | |
| is <strong>{activeId}</strong>. | |
| </p> | |
| </LayoutMain> | |
| </LayoutBody> | |
| </Layout> | |
| ); | |
| } | |
| export const ActiveItemTracking: Story = { | |
| render: () => <ActiveItemTrackingStory />, | |
| }; |
🧰 Tools
🪛 GitHub Check: build-and-test
[failure] 239-239:
React Hook "useState" is called in function "render" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/wordpress/Layout.stories.tsx` around lines 237 - 266, The
Story object ActiveItemTracking currently calls useState inside its render
function (named render) which violates the React hooks lint rule; extract the
stateful JSX into a separate React component (e.g., ActiveItemTrackingComponent)
that declares the hook (useState for activeId and setActiveId) and returns the
Layout tree, then change ActiveItemTracking.render to return
<ActiveItemTrackingComponent /> (or reference the new component) so the hook
lives in a properly capitalized component; update any inline handlers
(onItemClick) to use the component's setActiveId.
Comment on lines
+440
to
+444
| <p className="text-muted-foreground"> | ||
| The button above lives outside the Layout tree but toggles the sidebar | ||
| via <code>doAction("myapp_layout_toggle")</code>. Set the <code>namespace</code> prop | ||
| on Layout to control the hook name. | ||
| </p> |
There was a problem hiding this comment.
CI failure: Escape literal quote characters in JSX text.
Same unescaped " issue on line 442. The build/lint pipeline flags this as an error.
Proposed fix
<p className="text-muted-foreground">
The button above lives outside the Layout tree but toggles the sidebar
- via <code>doAction("myapp_layout_toggle")</code>. Set the <code>namespace</code> prop
+ via <code>doAction("myapp_layout_toggle")</code>. Set the <code>namespace</code> prop
on Layout to control the hook name.
</p>📝 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.
Suggested change
| <p className="text-muted-foreground"> | |
| The button above lives outside the Layout tree but toggles the sidebar | |
| via <code>doAction("myapp_layout_toggle")</code>. Set the <code>namespace</code> prop | |
| on Layout to control the hook name. | |
| </p> | |
| <p className="text-muted-foreground"> | |
| The button above lives outside the Layout tree but toggles the sidebar | |
| via <code>doAction("myapp_layout_toggle")</code>. Set the <code>namespace</code> prop | |
| on Layout to control the hook name. | |
| </p> |
🧰 Tools
🪛 GitHub Check: build-and-test
[failure] 442-442:
" can be escaped with ", “, ", ”
[failure] 442-442:
" can be escaped with ", “, ", ”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/wordpress/Layout.stories.tsx` around lines 440 - 444, The JSX
text contains raw double-quote characters causing lint/CI failures; update the
paragraph in Layout.stories.tsx so the quotes are escaped (e.g., change
doAction("myapp_layout_toggle") to doAction("myapp_layout_toggle") and
change <code>namespace</code> if it contains literals with quotes similarly) or
alternatively render the quoted strings using JS string nodes (e.g.,
{'"myapp_layout_toggle"'}); modify the <p> content where the text and
<code>doAction("myapp_layout_toggle")</code> appears to use " or {'"'} for
the quotes so the JSX no longer contains unescaped " characters.
Comment on lines
+247
to
+252
| export const LayoutSidebar = forwardRef<HTMLElement, LayoutSidebarProps>( | ||
| ({ className, children, side = "left", collapsible = "icon", variant = "sidebar", ...props }, ref) => { | ||
| return ( | ||
| <Sidebar | ||
| ref={ref as React.Ref<HTMLDivElement>} | ||
| data-slot="layout-sidebar" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# First, verify React namespace usage and cast pattern in the file
echo "=== Checking React.Ref usage and forwardRef pattern in layout.tsx ==="
rg -n 'React\.Ref|ref as React\.Ref|forwardRef<HTMLElement' src/components/wordpress/layout.tsx || echo "No matches found"
# Check the React imports at the top of the file
echo -e "\n=== Checking React imports ==="
head -20 src/components/wordpress/layout.tsx
# Check the LayoutSidebarProps interface definition
echo -e "\n=== Checking LayoutSidebarProps interface ==="
rg -n 'interface LayoutSidebarProps' -A 3 src/components/wordpress/layout.tsx
# Verify Sidebar primitive ref target generic
echo -e "\n=== Checking Sidebar component definition ==="
rg -n 'const\s+Sidebar\s*=|forwardRef' src/components/ui/sidebar.tsx -C 2 | head -30Repository: getdokan/plugin-ui
Length of output: 1189
Fix LayoutSidebar ref typing — React.Ref is not in scope.
Line 251 uses React.Ref<HTMLDivElement> but React is not imported (only named imports from "react" are used). This causes a TypeScript compilation error. Additionally, the cast masks a generic mismatch: forwardRef<HTMLElement, ...> declares HTMLElement but casts to HTMLDivElement. Fix by importing type Ref and aligning the generic:
Proposed fix
import {
createContext,
forwardRef,
useContext,
useEffect,
type HTMLAttributes,
type ReactNode,
+ type Ref,
} from "react";
@@
-export interface LayoutSidebarProps extends HTMLAttributes<HTMLElement> {
+export interface LayoutSidebarProps extends HTMLAttributes<HTMLDivElement> {
@@
-export const LayoutSidebar = forwardRef<HTMLElement, LayoutSidebarProps>(
+export const LayoutSidebar = forwardRef<HTMLDivElement, LayoutSidebarProps>(
({ className, children, side = "left", collapsible = "icon", variant = "sidebar", ...props }, ref) => {
return (
<Sidebar
- ref={ref as React.Ref<HTMLDivElement>}
+ ref={ref as Ref<HTMLDivElement>}
data-slot="layout-sidebar"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/wordpress/layout.tsx` around lines 247 - 252, LayoutSidebar's
ref typing is wrong and uses React.Ref without importing React and mismatches
HTMLElement vs HTMLDivElement; change the forwardRef generic from HTMLElement to
HTMLDivElement and import the type Ref from "react" (e.g., import type {
forwardRef, Ref } from "react") then cast the ref passed into Sidebar as
Ref<HTMLDivElement> (or better, keep ref typed as the forwarded HTMLDivElement)
so the forwardRef generic, the ref cast in the LayoutSidebar component, and the
Sidebar ref type all align.
mrabbani
changed the base branch from
main
to
feature/show-icon-minimize-sidebar
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Sidebarprimitives (SidebarProvider,Sidebar,SidebarInset,SidebarTrigger,SidebarMenu, etc.)src/components/ui/tosrc/components/wordpress/to separate WordPress-specific wrappers from base UI primitivessheet.tsxandsidebar.tsxas new shadcn base components, plususeIsMobilehookLayoutMenuAPI (items/groupsprops) — no breaking changes for consumersKey changes
SidebarProvider+Sidebarhandles responsive, mobile drawer, collapsed icon modesidebarPosition/sidebarVariant/sidebarBreakpointpropsside/collapsible/variantprops directly onLayoutSidebardata-mountedattribute suppresses transitions until after first paintNew exports
Sheet,SheetContent,SheetTrigger,SheetClose,SheetHeader,SheetTitle,SheetDescriptionSidebar,SidebarProvider,SidebarContent,SidebarGroup,SidebarMenu,SidebarMenuItem,SidebarMenuButton,SidebarMenuSub,SidebarHeader,SidebarFooter,SidebarTrigger,SidebarInset,SidebarRail,SidebarInput,useSidebar,useSidebarOptionaluseIsMobileTest plan
npx tsc --noEmitpasses with zero errorsdoAction("namespace_layout_toggle"))🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Documentation