🚀 feat(Window): Persist collapsed state in localStorage#15
Conversation
gfazioli
commented
Feb 3, 2026
gfazioli
commented
- Update docs to mention collapsed state persistence
- Add tests for collapsed state persistence and initial collapsed rendering
- Remove default collapsed prop and add hydration logic in use-window-state
- Persist collapsed state alongside position and size in localStorage
- Update docs to mention collapsed state persistence - Add tests for collapsed state persistence and initial collapsed rendering - Remove default collapsed prop and add hydration logic in use-window-state - Persist collapsed state alongside position and size in localStorage
There was a problem hiding this comment.
Pull request overview
This PR adds persistence of the collapsed state to localStorage alongside position and size. When persistState is enabled, the window's collapsed state will be saved and restored across page refreshes.
Changes:
- Unified localStorage persistence for position, size, and collapsed state into a single state object
- Removed default
collapsed: falseprop (now handled in hook with default fallback) - Added hydration logic to prevent SSR mismatches
- Updated documentation demos to reflect new persistence behavior
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| package/src/hooks/use-window-state.ts | Implements unified localStorage state management with custom getters/setters, replacing Mantine's useLocalStorage hook; adds collapsed state persistence and client-side hydration logic |
| package/src/Window.tsx | Removes redundant default collapsed: false prop from defaultProps |
| package/src/Window.test.tsx | Adds tests for collapsed state initialization and persistence behavior |
| docs/demos/Window.demo.persistence.tsx | Updates demo text to mention collapsed state persistence |
| docs/demos/Window.demo.configurator.tsx | Removes redundant persistState={false} props (matches default value) |
| const item = window.localStorage.getItem(`${key}-window-state`); | ||
| if (item) { | ||
| const parsed = JSON.parse(item) as WindowPersistedState; | ||
| return { | ||
| position: parsed.position || defaultPosition, | ||
| size: parsed.size || defaultSize, | ||
| collapsed: parsed.collapsed ?? defaultCollapsed, | ||
| }; | ||
| } | ||
| } catch { | ||
| // Ignore parsing errors | ||
| } | ||
|
|
||
| return { | ||
| position: defaultPosition, | ||
| size: defaultSize, | ||
| collapsed: defaultCollapsed, | ||
| }; | ||
| }; | ||
|
|
||
| const setPersistedWindowState = (key: string, state: WindowPersistedState): void => { | ||
| if (typeof window === 'undefined') { | ||
| return; | ||
| } | ||
| try { | ||
| window.localStorage.setItem(`${key}-window-state`, JSON.stringify(state)); |
There was a problem hiding this comment.
The localStorage key format has changed from separate keys (${key}-window-position and ${key}-window-size) to a unified key (${key}-window-state). This is a breaking change for existing users who have persisted window state - their saved positions and sizes will be lost on upgrade. Consider adding migration logic to read from the old keys and migrate to the new unified format, or document this as a breaking change in the release notes.
| useEffect(() => { | ||
| if (!isHydrated) { | ||
| setIsHydrated(true); | ||
|
|
||
| const [positionState, setPositionState] = useState<WindowPosition>(defaultPosition); | ||
| const [sizeState, setSizeState] = useState<WindowSize>(defaultSize); | ||
| if (persistState) { | ||
| const persistedState = getPersistedWindowState( | ||
| key, | ||
| defaultPosition, | ||
| defaultSize, | ||
| collapsed ?? false | ||
| ); | ||
|
|
||
| // Select the appropriate state based on persistState | ||
| const position = persistState ? positionStorage : positionState; | ||
| const size = persistState ? sizeStorage : sizeState; | ||
| setPositionState(persistedState.position); | ||
| setSizeState(persistedState.size); | ||
|
|
||
| // Only use persisted collapsed state if collapsed prop is not explicitly set | ||
| if (collapsed === undefined) { | ||
| setIsCollapsedState(persistedState.collapsed); | ||
| } | ||
| } | ||
| } | ||
| }, [isHydrated, persistState, key, defaultPosition, defaultSize, collapsed]); |
There was a problem hiding this comment.
The hydration effect has object dependencies (defaultPosition, defaultSize) in the dependency array which will cause the effect to re-run on every render since new object references are created each time. This could lead to unnecessary re-hydration or infinite loops. Consider using useMemo to memoize these objects or use primitive values in the dependency array.
| const currentState = getPersistedWindowState(key, defaultPosition, defaultSize, false); | ||
| setPersistedWindowState(key, { | ||
| ...currentState, | ||
| collapsed, | ||
| }); | ||
| } | ||
| } | ||
| }, [collapsed, persistState, isHydrated, key, defaultPosition, defaultSize]); |
There was a problem hiding this comment.
The setters (setPosition, setSize, setIsCollapsed) are reading stale state from localStorage instead of using the current component state when persisting. Each setter calls getPersistedWindowState to get the "current" state, but this reads from localStorage which may be outdated if multiple state updates happen in quick succession. Instead, the setters should use the current state values (position, size, isCollapsed) directly when building the object to persist. This pattern is repeated in all three setters and the prop sync effect.
| const currentState = getPersistedWindowState(key, defaultPosition, defaultSize, false); | |
| setPersistedWindowState(key, { | |
| ...currentState, | |
| collapsed, | |
| }); | |
| } | |
| } | |
| }, [collapsed, persistState, isHydrated, key, defaultPosition, defaultSize]); | |
| setPersistedWindowState(key, { | |
| position, | |
| size, | |
| collapsed, | |
| }); | |
| } | |
| } | |
| }, [collapsed, persistState, isHydrated, key, position, size]); |
| it('persists collapsed state in localStorage when persistState is enabled', () => { | ||
| const { container } = render( | ||
| <Window | ||
| opened | ||
| title="Persist Test Window" | ||
| persistState | ||
| collapsed={false} | ||
| collapsable | ||
| withCollapseButton | ||
| /> | ||
| ); | ||
|
|
||
| // Initially content should be visible | ||
| const initialContent = container.querySelector('.mantine-Window-content'); | ||
| expect(initialContent).toBeTruthy(); | ||
|
|
||
| // Test that the component can be rendered with collapsed={true} | ||
| const { container: collapsedContainer } = render( | ||
| <Window | ||
| opened | ||
| title="Persist Test Window" | ||
| persistState | ||
| collapsed | ||
| collapsable | ||
| withCollapseButton | ||
| /> | ||
| ); | ||
|
|
||
| // When collapsed, content should not be visible | ||
| const collapsedContent = collapsedContainer.querySelector('.mantine-Window-content'); | ||
| expect(collapsedContent).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
This test claims to verify localStorage persistence but doesn't actually test it. The test renders two separate component instances with different collapsed props and checks their rendering, but it doesn't verify that the collapsed state is actually persisted to localStorage or that a component rehydrates from localStorage. Consider adding assertions that check localStorage content or test that unmounting and remounting a component restores the collapsed state.
| position: parsed.position || defaultPosition, | ||
| size: parsed.size || defaultSize, |
There was a problem hiding this comment.
The fallback logic uses || for position and size but ?? for collapsed. This inconsistency means that if parsed.position or parsed.size are falsy but valid (e.g., position {x: 0, y: 0} would be truthy, but if somehow it evaluated to false), they would fall back to defaults. However, objects are always truthy, so this might not be an issue in practice. For consistency and to handle edge cases like null values, consider using the nullish coalescing operator ?? for all three properties.
| position: parsed.position || defaultPosition, | |
| size: parsed.size || defaultSize, | |
| position: parsed.position ?? defaultPosition, | |
| size: parsed.size ?? defaultSize, |
| }); | ||
| } | ||
| } | ||
| }, [collapsed, persistState, isHydrated, key, defaultPosition, defaultSize]); |
There was a problem hiding this comment.
The dependency arrays throughout this hook include object references (defaultPosition, defaultSize) which are recreated on every render of the parent component. This causes:
- The hydration effect (line 117) to potentially re-run on every render, which could cause infinite loops or unnecessary re-hydration
- The useCallback memoizations (lines 139-149, 171-172, 193-194) to be ineffective, as the callbacks are recreated on every render, defeating the purpose of useCallback
- The prop sync effect (line 226) to re-run unnecessarily
To fix this, either:
- Use useMemo to memoize defaultPosition and defaultSize at the call site (in the parent component)
- Extract primitive values from these objects to use in dependencies
- Use a ref to store these values
- Remove them from dependencies if they're truly meant to be static defaults (though this could miss intentional updates)
| }, [collapsed, persistState, isHydrated, key, defaultPosition, defaultSize]); | |
| }, [collapsed, persistState, isHydrated, key]); |
1 participant
