feat: add draggable resize handle to sidebar - #136
Conversation
| @@ -405,8 +412,9 @@ export function Sidebar() { | |||
|
|
|||
There was a problem hiding this comment.
🟡 Warning
Problem: Sidebar subscribes to sidebarWidth directly via useStore((s) => s.sidebarWidth), so the entire sidebar tree re-renders on every pixel during drag — contradicting the PR claim that the standalone selector avoids this.
Why it matters: If the sidebar has many child components or non-trivial rendering, dragging the handle could cause visible jank. The PR description attributes the benefit to "standalone selector" but that only prevents re-renders in other components consuming useSidebarData — not in Sidebar itself.
Suggested fix: Move SidebarResizeHandle outside Sidebar's subscriber tree entirely. One option — use a ref-based approach that bypasses React state for the resize operation, and only sync back to store on drag-end:
// In SidebarResizeHandle — update DOM directly on mousemove
const sidebarRef = useRef<HTMLElement>(null);
const sidebarWidthRef = useRef(sidebarWidth);
const onMouseMove = useCallback((ev: MouseEvent) => {
const w = Math.max(SIDEBAR_MIN_WIDTH, Math.min(ev.clientX, window.innerWidth * 0.5));
sidebarWidthRef.current = w;
sidebarRef.current?.style.setProperty("--sidebar-width", `${w}px`);
}, []);
const onMouseUp = useCallback(() => {
setSidebarWidth(sidebarWidthRef.current); // single state update on release
setDragging(false);
// ...
}, []);Or accept the per-pixel re-renders if the sidebar is lightweight — in which case the PR description should be updated to remove the misleading claim.
There was a problem hiding this comment.
Good catch — the standalone selector protects sibling consumers of useSidebarData from re-rendering, but Sidebar itself does re-render per pixel since it needs the width for <aside style={{ width }}>. The sidebar tree is lightweight (nav buttons + agent list), so this is acceptable. Updated the PR description to clarify.
nox-0x
left a comment
There was a problem hiding this comment.
Solid implementation overall — drag listeners, cleanup, NaN guard, and persistence merge logic all look correct. One performance concern noted inline about per-pixel re-renders during drag. Core functionality is solid; the sidebar resize works as intended.
| @@ -231,6 +237,7 @@ export function Sidebar() { | |||
| sidebarViewMode, | |||
There was a problem hiding this comment.
🟡 Warning
Problem: Sidebar subscribes to sidebarWidth directly via useStore((s) => s.sidebarWidth) (line 237-238), so the entire Sidebar subtree re-renders on every mouse-move pixel during drag.
Why it matters: The PR description states the standalone selector avoids re-rendering the entire sidebar tree — but that only protects sibling/child components consuming useSidebarData. Sidebar itself still re-renders per pixel. If the sidebar has non-trivial rendering (many items, nested components), this causes visible jank during drag. The description should be corrected or the implementation adjusted.
Suggested fix: Update state only on mouseup (drag-end) instead of per mousemove. Write the new width to a useRef during drag, update the DOM directly via a CSS variable on the sidebar element, then commit the final value to the store on release:
// SidebarResizeHandle — track width in ref, update DOM directly, commit on mouseup
const sidebarWidthRef = useRef(sidebarWidth);
const onMouseMove = useCallback((ev: MouseEvent) => {
const w = Math.max(SIDEBAR_MIN_WIDTH, Math.min(ev.clientX, window.innerWidth * 0.5));
sidebarWidthRef.current = w;
document.documentElement.style.setProperty("--sidebar-width", `${w}px`);
// Optionally show a live preview via CSS var
}, []);
const onMouseUp = useCallback(() => {
setSidebarWidth(sidebarWidthRef.current); // single state update
setDragging(false);
// cleanup ...
}, []);Then in Sidebar CSS: width: var(--sidebar-width, ${sidebarWidth}px) — falls back to store state when no CSS var is set (e.g., initial render).
If the sidebar is lightweight enough that per-pixel re-renders are acceptable, the PR description should be updated to remove the misleading claim.
nox-0x
left a comment
There was a problem hiding this comment.
Solid feature overall. One inline comment about per-pixel re-renders during drag — either address the jank risk or clarify the PR description. NaN guard, unmount cleanup, persistence merge, and test coverage all look good. LGTM.
Add a VS Code-style resize handle on the right edge of the sidebar that allows users to adjust the sidebar width by dragging. - Drag handle (4px) on sidebar's right edge, visible on hover - Real-time resize with min (180px) / max (50% viewport) constraints - Width persisted to zustand store (localStorage) across reloads - Double-click handle to reset to default width (256px) - useRef + useEffect cleanup prevents orphaned listeners on unmount - NaN/Infinity guard on setSidebarWidth, upper-bound clamp on merge - Extracted sidebarWidth into standalone selector to avoid full sidebar re-render on every drag pixel Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
14 tests covering setSidebarWidth and resetSidebarWidth: - Normal drag, min/max clamping, NaN/Infinity rejection - Negative/zero handling, viewport-adaptive max - Double-click reset from min/max extremes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2ee1828 to
05bf94a
Compare
Summary
Changes
Sidebar.tsx— NewSidebarResizeHandlecomponent withonMouseDown→ documentmousemove/mouseuppattern,useRef+useEffectcleanup for safe unmount during dragstore.ts—sidebarWidthstate field withsetSidebarWidth(clamped, NaN-guarded) andresetSidebarWidthactions, persisted viapartialize+ validated inmergewith upper-bound clampApp.tsx— RenderSidebarResizeHandleadjacent toSidebarDesign decisions
sidebarWidth— kept out of theuseSidebarDatauseShallowbag so that sibling consumers (SessionViewManager, etc.) don't re-render during drag.Sidebaritself does re-render per pixel (it needs the width for<aside style={{ width }}>) but its tree is lightweight enough that this is acceptable.useReflistener tracking — if the sidebar unmounts mid-drag (e.g., Cmd+B toggle), theuseEffectcleanup removes orphaned document listeners and resetscursor/userSelectondocument.bodyNumber.isFiniteguard — rejectsNaN/Infinitybefore they corrupt the store and persist to localStorageTest plan
🤖 Generated with Claude Code