feat: Add collapsible sidebar with persistent state and responsive grid#1116
feat: Add collapsible sidebar with persistent state and responsive grid#1116VasuS609 wants to merge 6 commits intoAOSSIE-Org:mainfrom
Conversation
- Added chevron toggle button in sidebar footer with smooth 180° rotation - Implemented persistent sidebar state using localStorage across app restarts - Added smooth transitions (300ms ease-in-out) for all sidebar animations - Footer text (version & copyright) displayed on single line, hidden when collapsed - Tooltip shows 'Collapse/Expand sidebar' on hover for better UX - Grid layout now responsive to sidebar state - shows more images per row when collapsed - Grid min-size adjusts from 224px to 200px when sidebar collapses for optimal space usage - All transitions synchronized and smooth without visual glitches
|
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a collapsible, persisted sidebar toggle and uses its state to adjust ChronologicalGallery's responsive grid min column size; updates app title and applies small code cleanups (predicate simplification and strict equality). Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Sidebar as AppSidebar
participant Store as SidebarStore (cookie)
participant Gallery as ChronologicalGallery
rect rgba(200,200,255,0.5)
Client->>Sidebar: Click toggle button
end
rect rgba(200,255,200,0.5)
Sidebar->>Store: write open state cookie (try/catch)
Store-->>Sidebar: persist confirmation
Sidebar->>Gallery: update open state via useSidebar
Gallery->>Gallery: compute gridMinSize (224px / 200px)
Gallery->>Client: re-render grid with new gridTemplateColumns
Client->>Sidebar: observe chevron rotation and footer collapse/expand
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
🤖 Fix all issues with AI agents
In `@frontend/src/components/Navigation/Sidebar/AppSidebar.tsx`:
- Line 37: The destructuring of useSidebar includes an unused variable `setOpen`
which causes lint/pipeline errors; update the statement `const { open, setOpen,
toggleSidebar } = useSidebar()` in AppSidebar.tsx to remove `setOpen` so it
simply destructures `open` and `toggleSidebar` from the `useSidebar()` hook.
🧹 Nitpick comments (2)
frontend/src/components/Navigation/Sidebar/AppSidebar.tsx (2)
40-53: Consider usingsetOpendirectly to avoid race condition.There's a subtle race condition between the load and save effects:
- On mount, save effect runs with the hook's initial
openvalue before the load effect's toggle takes effect.- This briefly overwrites the user's saved preference.
Using
setOpendirectly would be cleaner and avoid the issue:♻️ Proposed refactor
+ const { open, setOpen, toggleSidebar } = useSidebar(); + // Load collapsed state from localStorage on mount useEffect(() => { const savedState = localStorage.getItem('sidebar-open'); if (savedState !== null) { - const shouldBeOpen = savedState === 'true'; - if (open !== shouldBeOpen) { - toggleSidebar(); - } + setOpen(savedState === 'true'); } - }, []); + }, [setOpen]); // Save collapsed state to localStorage whenever it changes useEffect(() => { localStorage.setItem('sidebar-open', String(open)); }, [open]);Note: This approach would require keeping
setOpenin the destructuring (reverting the fix for the unused variable warning), but it's a more robust solution.
123-123: Consider using dynamic year for copyright.The hardcoded "2025" will need manual updates each year.
♻️ Proposed refactor
- <div className="whitespace-nowrap">© 2025 PictoPy</div> + <div className="whitespace-nowrap">© {new Date().getFullYear()} PictoPy</div>
- Implement collapsible sidebar with localStorage persistence - Add responsive gallery grid that adjusts to sidebar state (224px when open, 200px when closed) - Fix race condition in sidebar state management by using setOpen directly - Make copyright year dynamic using new Date().getFullYear() - Remove redundant boolean comparison in favorites filter - Update HTML title to 'PictoPy - AI-Powered Photo Manager' - Fix strict equality operator in landing page Fixes AOSSIE-Org#942
|
|
1 similar comment
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/components/Media/ChronologicalGallery.tsx`:
- Around line 38-42: Tests rendering Home and AITagging fail because
ChronologicalGallery calls useSidebar() which requires being inside
SidebarProvider; update allPages.test.tsx to wrap the rendered pages with the
SidebarProvider used in production. Specifically, modify the test render helpers
(or individual renders) to wrap <Home /> and <AITagging /> (the components that
render ChronologicalGallery) with <SidebarProvider> so useSidebar() no longer
throws; ensure you import the SidebarProvider from the same module used by
Layout and update any wrapper/setup utilities used across tests.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/components/Navigation/Sidebar/AppSidebar.tsx`:
- Around line 97-141: Remove the Git conflict markers and keep the
sidebar-toggle implementation (the block that uses open, toggleSidebar,
Tooltip/TooltipTrigger/TooltipContent, Button and ChevronLeft) instead of the
simpler main branch content; delete the lines starting with <<<<<<<, =======,
and >>>>>>> and ensure the JSX uses the animated container with classes that
reference open and the toggle button that calls toggleSidebar and displays
PictoPy v{version} and the current year.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/components/ui/sidebar.tsx`:
- Around line 74-87: Read the cookie synchronously inside the useState
initializer instead of in the React.useEffect to avoid first-paint flicker:
compute initialOpen by checking typeof document !== 'undefined' then parsing
document.cookie for SIDEBAR_COOKIE_NAME and falling back to defaultOpen, and
pass that initialOpen to the state hook that ultimately calls _setOpen; remove
or keep a no-op effect (React.useEffect) that currently reads the cookie to
avoid duplicated logic. Ensure you reference SIDEBAR_COOKIE_NAME and use the
same state setter (_setOpen) so behavior is unchanged and SSR (document
undefined) is guarded.
🧹 Nitpick comments (1)
frontend/src/components/ui/sidebar.tsx (1)
99-101: Persist cookie fromopenstate, not fromsetOpencalls.
Ifopenis controlled or changed externally, the cookie won’t update, andsetOpencan write a value the parent later rejects. Consider syncing cookie writes in an effect that watchesopen.Suggested adjustment
const setOpen = React.useCallback( (value: boolean | ((value: boolean) => boolean)) => { const openState = typeof value === 'function' ? value(open) : value; if (setOpenProp) { setOpenProp(openState); } else { _setOpen(openState); } - - // This sets the cookie to keep the sidebar state. - try { - document.cookie = `${SIDEBAR_COOKIE_NAME}=${openState}; path=/; max-age=${SIDEBAR_COOKIE_MAX_AGE}`; - } catch {} }, [setOpenProp, open], ); + + React.useEffect(() => { + try { + document.cookie = `${SIDEBAR_COOKIE_NAME}=${open}; path=/; max-age=${SIDEBAR_COOKIE_MAX_AGE}`; + } catch {} + }, [open]);
| // Restore sidebar state from cookie on mount | ||
| React.useEffect(() => { | ||
| try { | ||
| const cookies = document.cookie | ||
| .split('; ') | ||
| .find((row) => row.startsWith(SIDEBAR_COOKIE_NAME + '=')); | ||
| if (cookies) { | ||
| const savedState = cookies.split('=')[1] === 'true'; | ||
| _setOpen(savedState); | ||
| } | ||
| } catch { | ||
| // Ignore cookie errors | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the sidebar.tsx file
find . -name "sidebar.tsx" -type fRepository: AOSSIE-Org/PictoPy
Length of output: 102
🏁 Script executed:
# Read the sidebar.tsx file to check the actual code at lines 74-87 and surrounding context
cat -n frontend/src/components/ui/sidebar.tsx | head -120 | tail -70Repository: AOSSIE-Org/PictoPy
Length of output: 2497
🏁 Script executed:
# Also check the full context around the useState and useEffect
cat -n frontend/src/components/ui/sidebar.tsx | grep -A 30 -B 5 "SIDEBAR_COOKIE_NAME"Repository: AOSSIE-Org/PictoPy
Length of output: 3322
Avoid first-paint flicker by initializing sidebar state from the cookie synchronously.
The useEffect at lines 74–87 runs after paint, so if a saved cookie differs from defaultOpen, the sidebar will briefly render in the wrong state before the effect updates it. Move the cookie read into the useState initializer instead, guarding against SSR environments where document is unavailable.
Suggested adjustment
- const [_open, _setOpen] = React.useState(defaultOpen);
+ const [_open, _setOpen] = React.useState(() => {
+ if (typeof document === 'undefined') return defaultOpen;
+ try {
+ const cookie = document.cookie
+ .split(';')
+ .map((c) => c.trim())
+ .find((row) => row.startsWith(`${SIDEBAR_COOKIE_NAME}=`));
+ return cookie ? cookie.split('=')[1] === 'true' : defaultOpen;
+ } catch {
+ return defaultOpen;
+ }
+ });
- React.useEffect(() => {
- try {
- const cookies = document.cookie
- .split('; ')
- .find((row) => row.startsWith(SIDEBAR_COOKIE_NAME + '='));
- if (cookies) {
- const savedState = cookies.split('=')[1] === 'true';
- _setOpen(savedState);
- }
- } catch {
- // Ignore cookie errors
- }
- }, []);🤖 Prompt for AI Agents
In `@frontend/src/components/ui/sidebar.tsx` around lines 74 - 87, Read the cookie
synchronously inside the useState initializer instead of in the React.useEffect
to avoid first-paint flicker: compute initialOpen by checking typeof document
!== 'undefined' then parsing document.cookie for SIDEBAR_COOKIE_NAME and falling
back to defaultOpen, and pass that initialOpen to the state hook that ultimately
calls _setOpen; remove or keep a no-op effect (React.useEffect) that currently
reads the cookie to avoid duplicated logic. Ensure you reference
SIDEBAR_COOKIE_NAME and use the same state setter (_setOpen) so behavior is
unchanged and SSR (document undefined) is guarded.
1 participant
Fixes: #942
Screen.Recording.2026-01-31.194938.mp4
Summary by CodeRabbit
New Features
Style
Other