feat: add sidebar resizer component#64
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a resizable sidebar and supporting UI/logic. New SidebarResizer component provides drag-to-resize behavior, a ResizeObserver to report width or null below the desktop breakpoint, and global mouse handlers. Sidebar gains an optional enableResize (default true), state-managed sidebarWidth applied as inline fixed sizing, conditional rendering of the resizer, and a refactored two-pane flex layout (sidebar + page). Existing open/close event wiring and persistence to state[name] and state[name + ":default-open"] remain. CSS adds --sidebar_desktop_breakpoint, resizer styles, a gradient edge, and a gooey-sidebar-bordered class. Suggested reviewers
🚥 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. 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: 4
🧹 Nitpick comments (8)
app/components/Sidebar.tsx (3)
53-53: Use strict equality for comparison.Prefer
!==over!=for type-safe comparisons.- if (state[name] != isOpen) { + if (state[name] !== isOpen) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/Sidebar.tsx` at line 53, The conditional in Sidebar.tsx uses a loose comparison; change the comparison in the if statement that checks state[name] against isOpen to use strict inequality (!==) instead of != so the check in the component (the variables state, name, and isOpen) is type-safe and avoids unwanted type coercion.
27-50: Direct DOM manipulation to toggle button visibility is an anti-pattern in React.Modifying
style.displayon elements by class name bypasses React's rendering model and can lead to inconsistencies. Consider lifting the button visibility state up or using a context/callback pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/Sidebar.tsx` around lines 27 - 50, The useEffect currently toggles button visibility via direct DOM manipulation in handleOpen/handleClose (document.getElementsByClassName and setting style.display), which is an anti-pattern; instead expose a React-driven boolean (e.g., isOpen state lifted to a parent or provided via Context) and have the button components read that state (or receive a prop/callback) to render conditionally based on name + "-button" semantics—remove document.getElementsByClassName usage from handleOpen/handleClose and replace the event handlers to only update setOpen (or call a context setter), then adjust the button components to show/hide via React rendering instead of style mutations.
129-131: Hardcoded breakpoint duplicates the CSS variable.The
1140pxbreakpoint is defined both here and in CSS as--sidebar_desktop_breakpoint. This creates a maintenance burden if the breakpoint changes.♻️ Suggested approach
Read the CSS variable similarly to how
SidebarResizer.tsxdoes:function isDesktop() { if (typeof window === "undefined") return false; const breakpoint = parseInt( getComputedStyle(document.documentElement).getPropertyValue("--sidebar_desktop_breakpoint"), 10 ) || 1140; return window.innerWidth >= breakpoint; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/Sidebar.tsx` around lines 129 - 131, Replace the hardcoded 1140px in isDesktop with logic that reads the CSS variable --sidebar_desktop_breakpoint (like SidebarResizer.tsx does): first guard for typeof window === "undefined", then getComputedStyle(document.documentElement).getPropertyValue("--sidebar_desktop_breakpoint"), parse it with parseInt(..., 10), fallback to 1140 if NaN, and finally compare window.innerWidth >= breakpoint before returning.app/styles/app.css (2)
303-303: CSS variable defined but can't be used in media queries.The
--sidebar_desktop_breakpointvariable is useful for JavaScript access, but CSS media queries don't supportvar(). The1140pxvalue is duplicated at lines 351 and 400. Consider adding a comment noting this is intentional.--sidebar_desktop_breakpoint: 1140px; + /* Note: Media queries at lines 351 and 400 must be manually kept in sync */ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/styles/app.css` at line 303, The CSS variable --sidebar_desktop_breakpoint is defined for JS but cannot be used inside CSS media queries, and the literal value 1140px is duplicated in the media query rules; update the stylesheet by adding an inline comment next to the --sidebar_desktop_breakpoint declaration (and optionally near the repeated media query values) clarifying that media queries don't accept var() and that the duplication of 1140px is intentional for JS use, so future maintainers won't attempt to replace the hardcoded media query values with var().
393-398: Universal selector with!importantmay impact performance.
body.gooey-sidebar-resizing *applies styles to every descendant element. On complex pages, this could cause layout thrashing. Consider scoping to specific containers if performance issues arise.body.gooey-sidebar-resizing .gooey-sidebar, body.gooey-sidebar-resizing .gooey-sidebar-resizer { cursor: col-resize !important; user-select: none !important; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/styles/app.css` around lines 393 - 398, The rule using the universal descendant selector "body.gooey-sidebar-resizing *" with "!important" can hurt performance by touching every descendant; change the selector to target only the sidebar elements (e.g., ".gooey-sidebar" and ".gooey-sidebar-resizer") instead of all descendants, remove or keep "!important" only if strictly necessary, and update the CSS rule names (replace "body.gooey-sidebar-resizing *" with "body.gooey-sidebar-resizing .gooey-sidebar, body.gooey-sidebar-resizing .gooey-sidebar-resizer") so cursor and user-select styles are applied to the intended elements only.app/components/SidebarResizer.tsx (3)
73-73: Consider adding touch support for mobile/tablet devices.The resizer only handles mouse events. While the resizer is hidden on mobile via CSS, tablet users in landscape mode might benefit from touch-based resizing with
onTouchStart,touchmove, andtouchendhandlers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/SidebarResizer.tsx` at line 73, The resizer only registers mouse events; add touch support by wiring touch handlers on the SidebarResizer component (e.g., add onTouchStart that calls a new handleTouchStart function) and implement handleTouchStart to mirror handleMouseDown logic: read touch.clientX, preventDefault, add document touchmove and touchend listeners that call the same move/finish logic you use for mouse (or factor shared move/finish helpers used by handleMouseDown/handleMouseMove/handleMouseUp), and ensure those document touch listeners are removed on touchend and during cleanup to avoid leaks; reference SidebarResizer, handleMouseDown, and the existing mouse move/up handlers so the touch flow reuses the same resizing behavior.
18-19: Type inconsistency indragRefinitialization.The ref is initialized with an object
{ active: false, startX: 0, startWidth: 0 }but typed to allownull. Consider removing| nullfrom the type or initializing withnullif that's the intended initial state.- const dragRef = useRef({ active: false, startX: 0, startWidth: 0 }); + const dragRef = useRef<{ active: boolean; startX: number; startWidth: number }>({ active: false, startX: 0, startWidth: 0 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/SidebarResizer.tsx` around lines 18 - 19, The dragRef initialization in SidebarResizer.tsx is inconsistent with its type: widthRef is useRef<number | null>(null) but dragRef is initialized with an object while the comment indicates it may allow null; fix by making the types consistent — either remove "| null" and declare dragRef as useRef<{active:boolean,startX:number,startWidth:number}>({ active: false, startX: 0, startWidth: 0 }) or initialize it to null and type it useRef<{active:boolean,startX:number,startWidth:number} | null>(null); update any usages of dragRef in functions like the resize handlers to account for null if you choose the nullable route.
48-50: Fragile DOM traversal to read initial width.
event.currentTarget.previousElementSiblingassumes a specific DOM structure. If the layout changes, this will break or return incorrect values. Consider passing the sidebar element ref or using a more explicit selector.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/SidebarResizer.tsx` around lines 48 - 50, The code in SidebarResizer.tsx uses fragile DOM traversal via event.currentTarget.previousElementSibling to initialize widthRef.current; change this to rely on an explicit sidebar element reference instead: add or accept a sidebarRef prop (a RefObject<HTMLElement>) and replace the previousElementSibling access with sidebarRef.current?.offsetWidth ?? minWidth when initializing widthRef.current; update any handler (e.g., the pointerdown handler that contains the widthRef.current assignment) to use that sidebarRef and fall back to minWidth if the ref is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/Sidebar.tsx`:
- Line 62: The useEffect in the Sidebar component references onChange but the
dependency array currently only includes isOpen, name, and state; update the
effect's dependency array to include onChange so the effect reruns when the
onChange prop changes, ensuring the closure captures the latest handler (locate
the useEffect in Sidebar.tsx that ends with "}, [isOpen, name, state])" and add
onChange to that array).
- Line 64: The current destructuring "let [sidebarDiv, pageDiv] = children"
assumes exactly two children and can produce undefined values; change to
normalize and guard the input by converting children to an array via
React.Children.toArray(children) (or manually checking Array.isArray(children)),
then destructure with safe defaults (e.g., sidebarDiv and pageDiv default to
empty React nodes) and/or validate with React.isValidElement before accessing
.props.children; update references to sidebarDiv and pageDiv (the variables used
where .children is accessed) to use the guarded values or optional chaining so
runtime errors are avoided.
In `@app/components/SidebarResizer.tsx`:
- Around line 26-29: The CSS variable parse for --sidebar_desktop_breakpoint in
the SidebarResizer.tsx code can yield NaN; update the breakpoint calculation
(the getComputedStyle(...).getPropertyValue("--sidebar_desktop_breakpoint") ->
parseInt(...) expression) to validate the parsed value and fall back to a
sensible default (e.g., 1024) when parseInt returns NaN or the property is
empty; ensure you use radix 10 and use Number.isNaN or isNaN to detect invalid
parse and assign the fallback so subsequent viewport comparisons don't fail.
- Around line 39-41: The component's effect cleanup only removes the CSS class
and can leave window-level listeners active if unmounted mid-drag; create a
listenersRef (useRef) to hold the mousemove and mouseup handler functions inside
SidebarResizer, assign listenersRef.current before calling
window.addEventListener in handleMouseDown, and in the useEffect cleanup remove
both window.removeEventListener for the stored mousemove and mouseup handlers
(and then clear listenersRef.current) in addition to removing the
"gooey-sidebar-resizing" class so no dangling listeners remain on unmount.
---
Nitpick comments:
In `@app/components/Sidebar.tsx`:
- Line 53: The conditional in Sidebar.tsx uses a loose comparison; change the
comparison in the if statement that checks state[name] against isOpen to use
strict inequality (!==) instead of != so the check in the component (the
variables state, name, and isOpen) is type-safe and avoids unwanted type
coercion.
- Around line 27-50: The useEffect currently toggles button visibility via
direct DOM manipulation in handleOpen/handleClose
(document.getElementsByClassName and setting style.display), which is an
anti-pattern; instead expose a React-driven boolean (e.g., isOpen state lifted
to a parent or provided via Context) and have the button components read that
state (or receive a prop/callback) to render conditionally based on name +
"-button" semantics—remove document.getElementsByClassName usage from
handleOpen/handleClose and replace the event handlers to only update setOpen (or
call a context setter), then adjust the button components to show/hide via React
rendering instead of style mutations.
- Around line 129-131: Replace the hardcoded 1140px in isDesktop with logic that
reads the CSS variable --sidebar_desktop_breakpoint (like SidebarResizer.tsx
does): first guard for typeof window === "undefined", then
getComputedStyle(document.documentElement).getPropertyValue("--sidebar_desktop_breakpoint"),
parse it with parseInt(..., 10), fallback to 1140 if NaN, and finally compare
window.innerWidth >= breakpoint before returning.
In `@app/components/SidebarResizer.tsx`:
- Line 73: The resizer only registers mouse events; add touch support by wiring
touch handlers on the SidebarResizer component (e.g., add onTouchStart that
calls a new handleTouchStart function) and implement handleTouchStart to mirror
handleMouseDown logic: read touch.clientX, preventDefault, add document
touchmove and touchend listeners that call the same move/finish logic you use
for mouse (or factor shared move/finish helpers used by
handleMouseDown/handleMouseMove/handleMouseUp), and ensure those document touch
listeners are removed on touchend and during cleanup to avoid leaks; reference
SidebarResizer, handleMouseDown, and the existing mouse move/up handlers so the
touch flow reuses the same resizing behavior.
- Around line 18-19: The dragRef initialization in SidebarResizer.tsx is
inconsistent with its type: widthRef is useRef<number | null>(null) but dragRef
is initialized with an object while the comment indicates it may allow null; fix
by making the types consistent — either remove "| null" and declare dragRef as
useRef<{active:boolean,startX:number,startWidth:number}>({ active: false,
startX: 0, startWidth: 0 }) or initialize it to null and type it
useRef<{active:boolean,startX:number,startWidth:number} | null>(null); update
any usages of dragRef in functions like the resize handlers to account for null
if you choose the nullable route.
- Around line 48-50: The code in SidebarResizer.tsx uses fragile DOM traversal
via event.currentTarget.previousElementSibling to initialize widthRef.current;
change this to rely on an explicit sidebar element reference instead: add or
accept a sidebarRef prop (a RefObject<HTMLElement>) and replace the
previousElementSibling access with sidebarRef.current?.offsetWidth ?? minWidth
when initializing widthRef.current; update any handler (e.g., the pointerdown
handler that contains the widthRef.current assignment) to use that sidebarRef
and fall back to minWidth if the ref is missing.
In `@app/styles/app.css`:
- Line 303: The CSS variable --sidebar_desktop_breakpoint is defined for JS but
cannot be used inside CSS media queries, and the literal value 1140px is
duplicated in the media query rules; update the stylesheet by adding an inline
comment next to the --sidebar_desktop_breakpoint declaration (and optionally
near the repeated media query values) clarifying that media queries don't accept
var() and that the duplication of 1140px is intentional for JS use, so future
maintainers won't attempt to replace the hardcoded media query values with
var().
- Around line 393-398: The rule using the universal descendant selector
"body.gooey-sidebar-resizing *" with "!important" can hurt performance by
touching every descendant; change the selector to target only the sidebar
elements (e.g., ".gooey-sidebar" and ".gooey-sidebar-resizer") instead of all
descendants, remove or keep "!important" only if strictly necessary, and update
the CSS rule names (replace "body.gooey-sidebar-resizing *" with
"body.gooey-sidebar-resizing .gooey-sidebar, body.gooey-sidebar-resizing
.gooey-sidebar-resizer") so cursor and user-select styles are applied to the
intended elements only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9a83e36-af87-4674-b8f0-d80a617ecdab
📒 Files selected for processing (4)
app/components/Sidebar.tsxapp/components/SidebarResizer.tsxapp/renderer.tsxapp/styles/app.css
Confidence Score: 5/5 - Safe to MergeSafe to merge — this PR correctly addresses sidebar resizer behavior through targeted, well-scoped fixes across three files. The correction in Key Findings:
Files requiring special attention
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/components/SidebarResizer.tsx (1)
73-73: Expose the handle as an accessible separator.This is an interactive
divwith onlyonMouseDown, so it can't be focused or resized from the keyboard. Please add separator semantics (role="separator",aria-orientation="vertical",tabIndex={0}) plus arrow-key handling so the control isn't mouse-only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/SidebarResizer.tsx` at line 73, The resizer div in SidebarResizer is currently mouse-only; update the element returned by SidebarResizer to expose accessible separator semantics (add role="separator", aria-orientation="vertical", tabIndex={0}) and implement keyboard handling (onKeyDown) that mirrors the mouse drag behavior: handle ArrowLeft/ArrowRight to decrement/increment the sidebar width by a small step, support Home/End to set min/max widths, and ensure focus styles are visible; reuse or call the existing handleMouseDown’s underlying resize logic (or extract it into a shared function used by handleMouseDown and the new onKeyDown) so keyboard actions update the same state and fire the same callbacks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/SidebarResizer.tsx`:
- Around line 23-25: The effect that returns early when resizing is disabled
should also clear any fixed width so the sidebar isn't left pinned; inside the
useEffect in SidebarResizer.tsx, when enabled is false (or when enableResize
flips to false) call onWidthChange(null) (or the prop callback that clears
sidebarWidth) instead of just returning, ensuring the saved sidebarWidth is
removed; update the useEffect handling around enabled/useEffect to invoke
onWidthChange(null) when disabling so Sidebar.tsx stops applying the inline
width.
---
Nitpick comments:
In `@app/components/SidebarResizer.tsx`:
- Line 73: The resizer div in SidebarResizer is currently mouse-only; update the
element returned by SidebarResizer to expose accessible separator semantics (add
role="separator", aria-orientation="vertical", tabIndex={0}) and implement
keyboard handling (onKeyDown) that mirrors the mouse drag behavior: handle
ArrowLeft/ArrowRight to decrement/increment the sidebar width by a small step,
support Home/End to set min/max widths, and ensure focus styles are visible;
reuse or call the existing handleMouseDown’s underlying resize logic (or extract
it into a shared function used by handleMouseDown and the new onKeyDown) so
keyboard actions update the same state and fire the same callbacks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1358135-05cd-4c27-a084-b38481d667bb
📒 Files selected for processing (3)
app/components/Sidebar.tsxapp/components/SidebarResizer.tsxapp/styles/app.css
✅ Files skipped from review due to trivial changes (2)
- app/styles/app.css
- app/components/Sidebar.tsx
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
Confidence Score: 1/5 - Blocking IssuesNot safe to merge — this PR introduces a fundamental CSS incompatibility where Key Findings:
Files requiring special attention
|
- rename to sidebarContainerStyles - use --sidebar-desktop-breakpoint everywhere - render SidebarResizer when enabled true
anish-work
force-pushed
the
feat/sidebar-resizer
branch
from
8890671 to
4ab10cc
Compare
Confidence Score: 2/5 - Changes NeededNot safe to merge — while this PR successfully introduces a useful Key Findings:
Files requiring special attention
|
Confidence Score: 3/5 - Review RecommendedNot safe to merge without review — while this PR introduces reasonable defensive improvements (SSR guard for Key Findings:
Files requiring special attention
|
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app/components/Sidebar.tsx (1)
92-94:⚠️ Potential issue | 🟡 MinorOnly apply the remembered width when resize mode is active on desktop.
sidebarWidthis used unconditionally here. BecauseSidebarResizeronly mounts on Lines 108-113 while the sidebar is open, breakpoint changes that happen while it is closed never clear the stored desktop width. Reopening on mobile then overrides the mobile widths fromapp/styles/app.css, and the same stale width also survivesenableResize={false}. If re-enabling resize should not restore the old value later, also clearsidebarWidthwhenenableResizeflips false.🛠️ Suggested fix
- const sidebarContainerStyles = sidebarWidth + const sidebarContainerStyles = + enableResize && isDesktop() && sidebarWidth !== null ? { width: sidebarWidth, minWidth: sidebarWidth, maxWidth: sidebarWidth } : undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/Sidebar.tsx` around lines 92 - 94, Only apply the remembered width when resize mode is active on desktop and clear it when resize is disabled: change the sidebarContainerStyles computation to use the stored sidebarWidth only if enableResize is true and the current breakpoint is desktop (e.g., isDesktop or similar breakpoint flag used alongside SidebarResizer), e.g., sidebarContainerStyles = (enableResize && isDesktop && sidebarWidth) ? { width: sidebarWidth, minWidth: sidebarWidth, maxWidth: sidebarWidth } : undefined; and add logic to clear the stored sidebarWidth when enableResize flips false (reset the state/remembered value in the effect or handler that watches enableResize) so stale desktop widths don't override mobile CSS or persist when resizing is disabled.app/components/SidebarResizer.tsx (1)
37-39:⚠️ Potential issue | 🟡 MinorRemove the window drag handlers in the unmount cleanup.
If the component unmounts mid-drag, the
mousemove/mouseuplisteners added on Lines 72-73 stay alive until the user releases the mouse. That leaves a live drag loop callingonWidthChangeafter the resizer is gone.🛠️ Suggested fix
import { useEffect, useRef } from "react"; @@ const widthRef = useRef<number | null>(null); const dragRef = useRef({ active: false, startX: 0, startWidth: 0 }); + const listenersRef = useRef<{ + move: (event: MouseEvent) => void; + up: () => void; + } | null>(null); @@ useEffect(() => { - return () => document.body.classList.remove("gooey-sidebar-resizing"); + return () => { + document.body.classList.remove("gooey-sidebar-resizing"); + if (listenersRef.current) { + window.removeEventListener("mousemove", listenersRef.current.move); + window.removeEventListener("mouseup", listenersRef.current.up); + listenersRef.current = null; + } + }; }, []); @@ function onMouseUp() { dragRef.current.active = false; document.body.classList.remove("gooey-sidebar-resizing"); window.removeEventListener("mousemove", onMouseMove); window.removeEventListener("mouseup", onMouseUp); + listenersRef.current = null; } + listenersRef.current = { move: onMouseMove, up: onMouseUp }; window.addEventListener("mousemove", onMouseMove); window.addEventListener("mouseup", onMouseUp);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/SidebarResizer.tsx` around lines 37 - 39, The cleanup in SidebarResizer's useEffect only removes the "gooey-sidebar-resizing" class but must also remove the window 'mousemove' and 'mouseup' listeners added during dragging; update the cleanup to call window.removeEventListener for the same event types and handler functions used when attaching them (e.g., the handlers referenced where you call window.addEventListener('mousemove', ...) and window.addEventListener('mouseup', ...)), or store those handler functions in stable refs so the cleanup can remove them, and also keep the existing document.body.classList.remove call.
🧹 Nitpick comments (1)
app/components/SidebarResizer.tsx (1)
56-63: Don't re-render the whole layout on every drag frame.
onWidthChangehere becomessetSidebarWidthinapp/components/Sidebar.tsx, so each mousemove re-rendersGooeySidebarand bothRenderedChildrenbranches. On heavier pages the resize will feel sticky. Prefer updating the sidebar width imperatively during the drag and only committing the final value onmouseup, or throttle updates withrequestAnimationFrame. Please verify this path in the React Profiler with a populated page tree.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/SidebarResizer.tsx` around lines 56 - 63, The onMouseMove handler calls onWidthChange (which becomes setSidebarWidth) on every mousemove causing full React re-renders (GooeySidebar and RenderedChildren); change this so dragging updates width imperatively and only commits to React on mouseup, or throttle updates using requestAnimationFrame: keep updating widthRef.current and directly set the sidebar DOM element's style (or CSS variable) inside onMouseMove using dragRef/current DOM refs, avoid calling onWidthChange there, then call onWidthChange/setSidebarWidth once on mouseup (or schedule the React update inside a rAF loop and cancel on mouseup); ensure handlers referenced are onMouseMove, onMouseUp, dragRef, widthRef, onWidthChange/setSidebarWidth, GooeySidebar and verify in React Profiler.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/components/Sidebar.tsx`:
- Around line 92-94: Only apply the remembered width when resize mode is active
on desktop and clear it when resize is disabled: change the
sidebarContainerStyles computation to use the stored sidebarWidth only if
enableResize is true and the current breakpoint is desktop (e.g., isDesktop or
similar breakpoint flag used alongside SidebarResizer), e.g.,
sidebarContainerStyles = (enableResize && isDesktop && sidebarWidth) ? { width:
sidebarWidth, minWidth: sidebarWidth, maxWidth: sidebarWidth } : undefined; and
add logic to clear the stored sidebarWidth when enableResize flips false (reset
the state/remembered value in the effect or handler that watches enableResize)
so stale desktop widths don't override mobile CSS or persist when resizing is
disabled.
In `@app/components/SidebarResizer.tsx`:
- Around line 37-39: The cleanup in SidebarResizer's useEffect only removes the
"gooey-sidebar-resizing" class but must also remove the window 'mousemove' and
'mouseup' listeners added during dragging; update the cleanup to call
window.removeEventListener for the same event types and handler functions used
when attaching them (e.g., the handlers referenced where you call
window.addEventListener('mousemove', ...) and window.addEventListener('mouseup',
...)), or store those handler functions in stable refs so the cleanup can remove
them, and also keep the existing document.body.classList.remove call.
---
Nitpick comments:
In `@app/components/SidebarResizer.tsx`:
- Around line 56-63: The onMouseMove handler calls onWidthChange (which becomes
setSidebarWidth) on every mousemove causing full React re-renders (GooeySidebar
and RenderedChildren); change this so dragging updates width imperatively and
only commits to React on mouseup, or throttle updates using
requestAnimationFrame: keep updating widthRef.current and directly set the
sidebar DOM element's style (or CSS variable) inside onMouseMove using
dragRef/current DOM refs, avoid calling onWidthChange there, then call
onWidthChange/setSidebarWidth once on mouseup (or schedule the React update
inside a rAF loop and cancel on mouseup); ensure handlers referenced are
onMouseMove, onMouseUp, dragRef, widthRef, onWidthChange/setSidebarWidth,
GooeySidebar and verify in React Profiler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 46116fb3-c513-459c-b523-87dd510dc6ec
📒 Files selected for processing (3)
app/components/Sidebar.tsxapp/components/SidebarResizer.tsxapp/styles/app.css
🚧 Files skipped from review as they are similar to previous changes (1)
- app/styles/app.css
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/components/Sidebar.tsx (1)
17-17:⚠️ Potential issue | 🟠 MajorGuard the two-pane contract before dereferencing
children.
childrenis typed asArray<TreeNode>, but both render branches assumesidebarDivandpageDivexist. Any malformed sidebar node with fewer than two children will throw the first time.childrenis read. A 2-tuple type plus a small runtime guard would make this contract explicit and fail closed.Also applies to: 64-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/Sidebar.tsx` at line 17, The component currently types props.children as Array<TreeNode> but dereferences two entries (sidebarDiv/pageDiv) without checking length; change the prop type to a 2-tuple (e.g., [TreeNode, TreeNode]) to make the contract explicit and add a runtime guard wherever children is used (check children && children.length >= 2) before reading children[0] and children[1]; if the guard fails, return a safe fallback (null or an error UI) so the code fails closed—apply the same change/guard around the other occurrence noted (the branch at the other use, e.g., lines referenced near sidebarDiv/pageDiv).app/components/SidebarResizer.tsx (1)
33-35:⚠️ Potential issue | 🟡 MinorTear down the window drag handlers from the unmount cleanup.
If the sidebar closes mid-drag, this cleanup only removes
gooey-sidebar-resizing. Themousemove/mouseuphandlers stay registered until the next mouseup and can keep callingonWidthChangeafter the control is gone. Store the active handlers in a ref and remove them from the effect cleanup too.Also applies to: 52-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/SidebarResizer.tsx` around lines 33 - 35, The effect cleanup in SidebarResizer currently only removes the "gooey-sidebar-resizing" class but does not detach the window mousemove/mouseup handlers, so if the sidebar unmounts mid-drag onWidthChange may keep being called; update the component (within the useEffect that adds the window.addEventListener handlers and the similar touch effect around the onPointer handlers) to store the listener functions in a ref (e.g., a dragHandlersRef) when attaching them and call window.removeEventListener for both "mousemove" and "mouseup" (and the touch equivalents) in the effect's return cleanup, and ensure the same ref-held functions are used for removal so handlers are fully torn down on unmount or when drag ends.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/SidebarResizer.tsx`:
- Around line 14-15: The component currently stores the last dragged width only
in the local widthRef, which is lost when SidebarResizer unmounts; on mount
rehydrate that ref from the parent-owned sidebarWidth prop so a close/reopen
cycle preserves the last width. Concretely, in SidebarResizer add a useEffect
that runs on mount (or when sidebarWidth prop changes) and sets widthRef.current
= sidebarWidth, and ensure that when dragging ends you still update the
parent-owned width (e.g., call the existing onSidebarWidthChange /
setSidebarWidth) so the parent keeps the persisted value to rehydrate later.
---
Duplicate comments:
In `@app/components/Sidebar.tsx`:
- Line 17: The component currently types props.children as Array<TreeNode> but
dereferences two entries (sidebarDiv/pageDiv) without checking length; change
the prop type to a 2-tuple (e.g., [TreeNode, TreeNode]) to make the contract
explicit and add a runtime guard wherever children is used (check children &&
children.length >= 2) before reading children[0] and children[1]; if the guard
fails, return a safe fallback (null or an error UI) so the code fails
closed—apply the same change/guard around the other occurrence noted (the branch
at the other use, e.g., lines referenced near sidebarDiv/pageDiv).
In `@app/components/SidebarResizer.tsx`:
- Around line 33-35: The effect cleanup in SidebarResizer currently only removes
the "gooey-sidebar-resizing" class but does not detach the window
mousemove/mouseup handlers, so if the sidebar unmounts mid-drag onWidthChange
may keep being called; update the component (within the useEffect that adds the
window.addEventListener handlers and the similar touch effect around the
onPointer handlers) to store the listener functions in a ref (e.g., a
dragHandlersRef) when attaching them and call window.removeEventListener for
both "mousemove" and "mouseup" (and the touch equivalents) in the effect's
return cleanup, and ensure the same ref-held functions are used for removal so
handlers are fully torn down on unmount or when drag ends.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6cfa9f7d-b179-4480-9733-5dabda86f451
📒 Files selected for processing (3)
app/components/Sidebar.tsxapp/components/SidebarResizer.tsxapp/styles/app.css
Confidence Score: 3/5 - Review RecommendedLikely safe but review recommended — this PR introduces controlled width state management for Key Findings:
Files requiring special attention
|
Confidence Score: 3/5 - Review RecommendedLikely safe but review recommended — this PR adds a simple Key Findings:
Files requiring special attention
|
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app/components/SidebarResizer.tsx (1)
16-17:⚠️ Potential issue | 🟠 MajorTear down active drag listeners on unmount.
SidebarResizeris unmounted whenever the sidebar closes inapp/components/Sidebar.tsxLines 108-115, but the cleanup here only removes the body class. If that happens mid-drag, themousemove/mouseuphandlers keep firing against a stale closure until the next mouseup.🛠️ Possible fix
export default function SidebarResizer({ minWidth = 340, maxWidth = 800, width, onWidthChange, }: Props) { const widthRef = useRef<number | null>(width); const dragRef = useRef({ active: false, startX: 0, startWidth: 0 }); + const listenersRef = useRef<{ + move: (event: MouseEvent) => void; + up: () => void; + } | null>(null); useEffect(() => { widthRef.current = width; }, [width]); @@ useEffect(() => { - return () => document.body.classList.remove("gooey-sidebar-resizing"); + return () => { + dragRef.current.active = false; + document.body.classList.remove("gooey-sidebar-resizing"); + if (listenersRef.current) { + window.removeEventListener("mousemove", listenersRef.current.move); + window.removeEventListener("mouseup", listenersRef.current.up); + listenersRef.current = null; + } + }; }, []); @@ function onMouseUp() { dragRef.current.active = false; document.body.classList.remove("gooey-sidebar-resizing"); window.removeEventListener("mousemove", onMouseMove); window.removeEventListener("mouseup", onMouseUp); + listenersRef.current = null; } + listenersRef.current = { move: onMouseMove, up: onMouseUp }; window.addEventListener("mousemove", onMouseMove); window.addEventListener("mouseup", onMouseUp); }Also applies to: 39-40, 58-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/SidebarResizer.tsx` around lines 16 - 17, The component SidebarResizer currently only removes the body class on cleanup but leaves global mousemove/mouseup handlers active; update the component (referencing SidebarResizer, widthRef, dragRef and the mouse event handler functions) to store the actual handler references when adding window/document listeners and remove those exact listeners in the useEffect cleanup and on unmount, and if dragRef.current.active is true clear it and remove the body class so no stale closures keep firing; ensure you call removeEventListener for both "mousemove" and "mouseup" using the same handler functions you added and reset dragRef.current.active to false.app/components/Sidebar.tsx (1)
108-115:⚠️ Potential issue | 🟠 MajorClosing on mobile still forgets the last dragged width.
app/components/SidebarResizer.tsxkeeps the previous desktop width in a localwidthRef, but it also writesnullinto parent state below the breakpoint. Because this block unmounts the resizer wheneverisOpenbecomes false, closing on mobile drops that ref and the next desktop open falls back to the default width instead of the user's last drag size. Persist the last desktop width inGooeySidebar, or keep the breakpoint-sync logic mounted independently of the handle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/Sidebar.tsx` around lines 108 - 115, The SidebarResizer unmounts when isOpen toggles false which drops the local widthRef and causes desktop reopen to lose the last dragged width; persist the last desktop width in the GooeySidebar component instead of keeping it only in SidebarResizer and ensure breakpoint sync logic remains mounted/decoupled from the resizer handle. Modify GooeySidebar to own the canonical lastDesktopWidth (initialize from sidebarWidth), update that value via the onWidthChange handler (setSidebarWidth) only when the window is >= desktop breakpoint, and read it back when opening on desktop; alternatively, keep the breakpoint-sync effect in GooeySidebar (not inside SidebarResizer) so closing the mobile UI does not discard the remembered widthRef. Ensure you reference SidebarResizer, widthRef, sidebarWidth, setSidebarWidth, GooeySidebar and isOpen when moving state/effect.
🧹 Nitpick comments (1)
app/components/SidebarResizer.tsx (1)
78-79: Expose the handle as an actual separator control.This is currently a mouse-only
div, so keyboard and assistive-tech users cannot discover or operate the resize feature. A focusable separator role plus arrow-key handling would make the new control accessible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/SidebarResizer.tsx` around lines 78 - 79, The resizer is currently an unfocusable div; update the SidebarResizer component so the visible handle (className "gooey-sidebar-resizer") is an interactive, accessible separator: render it with role="separator", tabIndex={0}, and appropriate ARIA attributes (aria-orientation, aria-valuemin, aria-valuemax, aria-valuenow or aria-label) and add an onKeyDown handler that mirrors handleMouseDown's behavior (handleLeft/Right or Up/Down arrow keys to decrement/increment width and Home/End to min/max) while keeping the existing handleMouseDown for pointer dragging; also ensure focus/hover styles are preserved so keyboard users can discover the control.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/components/Sidebar.tsx`:
- Around line 108-115: The SidebarResizer unmounts when isOpen toggles false
which drops the local widthRef and causes desktop reopen to lose the last
dragged width; persist the last desktop width in the GooeySidebar component
instead of keeping it only in SidebarResizer and ensure breakpoint sync logic
remains mounted/decoupled from the resizer handle. Modify GooeySidebar to own
the canonical lastDesktopWidth (initialize from sidebarWidth), update that value
via the onWidthChange handler (setSidebarWidth) only when the window is >=
desktop breakpoint, and read it back when opening on desktop; alternatively,
keep the breakpoint-sync effect in GooeySidebar (not inside SidebarResizer) so
closing the mobile UI does not discard the remembered widthRef. Ensure you
reference SidebarResizer, widthRef, sidebarWidth, setSidebarWidth, GooeySidebar
and isOpen when moving state/effect.
In `@app/components/SidebarResizer.tsx`:
- Around line 16-17: The component SidebarResizer currently only removes the
body class on cleanup but leaves global mousemove/mouseup handlers active;
update the component (referencing SidebarResizer, widthRef, dragRef and the
mouse event handler functions) to store the actual handler references when
adding window/document listeners and remove those exact listeners in the
useEffect cleanup and on unmount, and if dragRef.current.active is true clear it
and remove the body class so no stale closures keep firing; ensure you call
removeEventListener for both "mousemove" and "mouseup" using the same handler
functions you added and reset dragRef.current.active to false.
---
Nitpick comments:
In `@app/components/SidebarResizer.tsx`:
- Around line 78-79: The resizer is currently an unfocusable div; update the
SidebarResizer component so the visible handle (className
"gooey-sidebar-resizer") is an interactive, accessible separator: render it with
role="separator", tabIndex={0}, and appropriate ARIA attributes
(aria-orientation, aria-valuemin, aria-valuemax, aria-valuenow or aria-label)
and add an onKeyDown handler that mirrors handleMouseDown's behavior
(handleLeft/Right or Up/Down arrow keys to decrement/increment width and
Home/End to min/max) while keeping the existing handleMouseDown for pointer
dragging; also ensure focus/hover styles are preserved so keyboard users can
discover the control.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c787af5f-1e08-44e2-8b38-74551a4e36e9
📒 Files selected for processing (2)
app/components/Sidebar.tsxapp/components/SidebarResizer.tsx
|
@nikochiko let's merge now ? |
There was a problem hiding this comment.
♻️ Duplicate comments (3)
app/components/Sidebar.tsx (2)
52-62:⚠️ Potential issue | 🟡 MinorMissing
onChangein the dependency array.The effect calls
onChange()on line 60 but it's not listed in the dependency array, which could lead to stale closures.- }, [isOpen, name, state]); + }, [isOpen, name, state, onChange]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/Sidebar.tsx` around lines 52 - 62, Effect invokes onChange but it isn't in the dependency array; update the useEffect dependencies to include onChange (i.e. change [isOpen, name, state] to [isOpen, name, state, onChange]) and if onChange is not stable ensure it is memoized (e.g., wrap onChange in useCallback where it is defined) so useEffect doesn't re-run unnecessarily; verify useEffect logic with symbols useEffect, state[name], isOpen, isDesktop, and onChange after the change.
64-64:⚠️ Potential issue | 🟡 MinorUnsafe destructuring assumes
childrenhas exactly 2 elements.If
childrenhas fewer than 2 elements,sidebarDivorpageDivwill beundefined, causing runtime errors when accessing.childrenon lines 70, 104, and 120.🛡️ Proposed defensive check
let [sidebarDiv, pageDiv] = children; + + if (!sidebarDiv || !pageDiv) { + console.error("GooeySidebar requires exactly 2 children: sidebar and page"); + return null; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/Sidebar.tsx` at line 64, The current destructuring "let [sidebarDiv, pageDiv] = children" assumes children has exactly two elements and can produce undefined values used later; change to first normalize children with React.Children.toArray(children) and safely extract defaults (e.g. sidebarDiv=null, pageDiv=null) and then guard usages (use optional chaining or check React.isValidElement before accessing .props.children or .children) in the Sidebar component where sidebarDiv and pageDiv are referenced to avoid runtime errors.app/components/SidebarResizer.tsx (1)
39-41:⚠️ Potential issue | 🟡 MinorDangling event listeners if unmount occurs mid-drag.
The cleanup effect only removes the CSS class. If the component unmounts while dragging, the
mousemoveandmouseuplisteners attached towindowremain active, potentially causing state updates on an unmounted component.Store the listeners in a ref and remove them in the cleanup:
🛡️ Proposed fix
+ const listenersRef = useRef<{ + move: (e: MouseEvent) => void; + up: () => void; + } | null>(null); useEffect(() => { - return () => document.body.classList.remove("gooey-sidebar-resizing"); + return () => { + document.body.classList.remove("gooey-sidebar-resizing"); + if (listenersRef.current) { + window.removeEventListener("mousemove", listenersRef.current.move); + window.removeEventListener("mouseup", listenersRef.current.up); + listenersRef.current = null; + } + }; }, []);Then in
handleMouseDown, before adding listeners:+ listenersRef.current = { move: onMouseMove, up: onMouseUp }; window.addEventListener("mousemove", onMouseMove); window.addEventListener("mouseup", onMouseUp);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/SidebarResizer.tsx` around lines 39 - 41, The cleanup currently only removes the CSS class and can leave window mousemove/mouseup listeners attached if unmount happens mid-drag; modify SidebarResizer so handleMouseDown creates the actual listener functions and stores them in a ref (e.g., listenersRef), attach those functions to window, and in the useEffect cleanup remove both window.removeEventListener('mousemove', listenersRef.current.move) and window.removeEventListener('mouseup', listenersRef.current.up') as well as clearing the ref and removing the "gooey-sidebar-resizing" class; ensure the same function references used when adding are used when removing to avoid dangling listeners.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/components/Sidebar.tsx`:
- Around line 52-62: Effect invokes onChange but it isn't in the dependency
array; update the useEffect dependencies to include onChange (i.e. change
[isOpen, name, state] to [isOpen, name, state, onChange]) and if onChange is not
stable ensure it is memoized (e.g., wrap onChange in useCallback where it is
defined) so useEffect doesn't re-run unnecessarily; verify useEffect logic with
symbols useEffect, state[name], isOpen, isDesktop, and onChange after the
change.
- Line 64: The current destructuring "let [sidebarDiv, pageDiv] = children"
assumes children has exactly two elements and can produce undefined values used
later; change to first normalize children with React.Children.toArray(children)
and safely extract defaults (e.g. sidebarDiv=null, pageDiv=null) and then guard
usages (use optional chaining or check React.isValidElement before accessing
.props.children or .children) in the Sidebar component where sidebarDiv and
pageDiv are referenced to avoid runtime errors.
In `@app/components/SidebarResizer.tsx`:
- Around line 39-41: The cleanup currently only removes the CSS class and can
leave window mousemove/mouseup listeners attached if unmount happens mid-drag;
modify SidebarResizer so handleMouseDown creates the actual listener functions
and stores them in a ref (e.g., listenersRef), attach those functions to window,
and in the useEffect cleanup remove both window.removeEventListener('mousemove',
listenersRef.current.move) and window.removeEventListener('mouseup',
listenersRef.current.up') as well as clearing the ref and removing the
"gooey-sidebar-resizing" class; ensure the same function references used when
adding are used when removing to avoid dangling listeners.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c99117c6-1144-470e-bc7c-b4d5c058518f
📒 Files selected for processing (2)
app/components/Sidebar.tsxapp/components/SidebarResizer.tsx
|
doesn't seem to remember last resize position on reload |
3 participants
Uh oh!
There was an error while loading. Please reload this page.