diff --git a/src/client/Sidebar.tsx b/src/client/Sidebar.tsx index 8d4cafc..2cafd83 100644 --- a/src/client/Sidebar.tsx +++ b/src/client/Sidebar.tsx @@ -472,7 +472,13 @@ export function Sidebar(props: { ctx: Context; store: SidebarStore }) { // `width + detailsWidth` — derived from the measured column, keeping the // drag write-only (no React re-render mid-drag). bottomRef.current?.style.setProperty('right', `${(window.innerWidth - centerRect.right) + (width - (state?.width ?? 0))}px`) - document.documentElement.style.setProperty('--dsh-sidebar-width', `${width}px`) + // The width push exists only while the right panel is OPEN (the layout + // push effect writes 0 while it is collapsed). The bottom-height drag + // reuses this function with the right panel's PERSISTED width as `width`, + // so writing it unconditionally would squeeze the whole app shell left + // mid-drag — as if the collapsed right panel were still occupying space — + // until the store commit on release snaps it back. + document.documentElement.style.setProperty('--dsh-sidebar-width', `${state?.panelOpen === true ? width : 0}px`) document.documentElement.style.setProperty('--dsh-sidebar-height', `${height}px`) if (cornerRef.current !== null) { cornerRef.current.style.left = `${window.innerWidth - width - 6}px` @@ -516,6 +522,18 @@ export function Sidebar(props: { ctx: Context; store: SidebarStore }) { // crush the app shell to zero. Dragging disables the layout transition. // On NARROW viewports the drawer FLOATS over the app shell — no push, the // conversation keeps the full width behind the drawer. + // + // This effect is deliberately SET-only (no cleanup): React runs every + // effect cleanup before every effect setup in the same commit, so a + // cleanup here would remove both variables before the draggingRef effect + // (declared above) runs its setup and forces a layout in measureCenter + // (getBoundingClientRect). That forced recalc would resolve #root's + // margin-right to the 0px fallback, cache it as the transition start + // value, and measure centerRect far too wide; once the body attribute + // effect re-enables transitions on pointer-up, the shell would animate + // margin-right 0 → width — the visible "expand to the full page then + // bounce back" flash. Unmount release lives in the effect below + // (issue #31). useEffect(() => { const width = !narrow && snapshot.state?.panelOpen === true ? Math.min(snapshot.state.width, window.innerWidth) @@ -525,18 +543,20 @@ export function Sidebar(props: { ctx: Context; store: SidebarStore }) { : 0 document.documentElement.style.setProperty('--dsh-sidebar-width', `${width}px`) document.documentElement.style.setProperty('--dsh-sidebar-height', `${height}px`) - // Unmount must release the push (issue #31): when the boundary swaps the - // whole sidebar after a render crash (or the plugin fiber is disposed / - // HMR), the CSS variables would otherwise stay on and layout.css - // keeps squeezing #root with a stale margin — "the sidebar cannot be - // hidden" until a full reload. removeProperty restores the CSS fallback - // (var(--dsh-sidebar-width, 0px)); React re-runs cleanup+setup in the - // same commit on state changes, so there is no visible flicker. + }, [narrow, snapshot.state?.panelOpen, snapshot.state?.width, snapshot.state?.bottomOpen, snapshot.state?.bottomHeight]) + // Unmount must release the push (issue #31): when the boundary swaps the + // whole sidebar after a render crash (or the plugin fiber is disposed / + // HMR), the CSS variables would otherwise stay on and layout.css + // keeps squeezing #root with a stale margin — "the sidebar cannot be + // hidden" until a full reload. removeProperty restores the CSS fallback + // (var(--dsh-sidebar-width, 0px)). Kept separate from the set effect so a + // dep-change re-run never removes the variables mid-commit. + useEffect(() => { return () => { document.documentElement.style.removeProperty('--dsh-sidebar-width') document.documentElement.style.removeProperty('--dsh-sidebar-height') } - }, [narrow, snapshot.state?.panelOpen, snapshot.state?.width, snapshot.state?.bottomOpen, snapshot.state?.bottomHeight]) + }, []) useEffect(() => { if (anyDragging) document.body.setAttribute('data-dsh-sidebar-dragging', '') else document.body.removeAttribute('data-dsh-sidebar-dragging') diff --git a/tests/sidebar-crash.spec.tsx b/tests/sidebar-crash.spec.tsx index a6c93f4..7adb552 100644 --- a/tests/sidebar-crash.spec.tsx +++ b/tests/sidebar-crash.spec.tsx @@ -26,7 +26,7 @@ import { act } from 'react-dom/test-utils' ;(globalThis as Record).IS_REACT_ACT_ENVIRONMENT = true import { Sidebar } from '../src/client/Sidebar.tsx' -import { createSidebarStore, type SidebarStore } from '../src/client/state.ts' +import { createSidebarStore, setBottomHeight, toggleBottomPanel, type SidebarStore } from '../src/client/state.ts' import { createBetterSidebarService, type BetterSidebarService } from '../src/client/service.ts' import { t } from '../src/client/locales.ts' @@ -99,6 +99,36 @@ describe('layout-push variable cleanup', () => { expect(htmlStyle.getPropertyValue('--dsh-sidebar-width')).toBe('') expect(htmlStyle.getPropertyValue('--dsh-sidebar-height')).toBe('') }) + + it('a size change (release commit) re-applies the variables without removing them mid-commit', () => { + const { store, unmount } = mountSidebar() + const htmlStyle = document.documentElement.style + const width = store.getSnapshot().state!.width + const removeSpy = vi.spyOn(htmlStyle, 'removeProperty') + // Simulate a drag release: the bottom panel opens and its height commits. + act(() => { store.reduce(toggleBottomPanel) }) + act(() => { store.reduce(s => setBottomHeight(s, 300)) }) + expect(htmlStyle.getPropertyValue('--dsh-sidebar-width')).toBe(`${width}px`) + expect(htmlStyle.getPropertyValue('--dsh-sidebar-height')).toBe('300px') + // The commit must NOT have removed the variables at any point. React + // runs every effect cleanup before every effect setup in a commit, so a + // cleanup here would remove the variables before the draggingRef effect + // (declared above the layout push) forces a layout in measureCenter + // (getBoundingClientRect). That forced recalc resolves #root's + // margin-right to the 0px fallback, caches it as the transition start + // value, and measures centerRect too wide; once transitions re-enable on + // release, the shell animates margin-right 0 → width — the "expand to + // the full page then bounce back" flash. + const removalProps = removeSpy.mock.calls + .map(call => call[0] as string) + .filter(prop => prop === '--dsh-sidebar-width' || prop === '--dsh-sidebar-height') + expect(removalProps).toHaveLength(0) + // Only unmounting may remove them (issue #31). + unmount() + expect(removeSpy.mock.calls.some(call => + call[0] === '--dsh-sidebar-width' || call[0] === '--dsh-sidebar-height')).toBe(true) + removeSpy.mockRestore() + }) }) describe('tab crash containment', () => {