fix:Fix merge Conflict#1291
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
- 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
WalkthroughSidebar state is now persisted via a browser cookie and consumed by AppSidebar and ChronologicalGallery to drive animated footer behavior, chevron rotation, menu transitions, and a responsive image grid. Small independent edits update favorites filtering and the document title. ChangesSidebar state persistence and responsive layout
Independent fixes and branding
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/ui/sidebar.tsx (1)
73-103: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftAdd automated tests for sidebar cookie persistence paths.
Line 75-87 and Line 99-101 introduce critical state persistence behavior, but this PR doesn’t show automated coverage for restore/write/error-tolerant paths (including controlled vs uncontrolled mode). Please add focused tests for these flows before merge.
As per coding guidelines, "Ensure that test code is automated, comprehensive, and follows testing best practices" and "Verify that all critical functionality is covered by tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/ui/sidebar.tsx` around lines 73 - 103, Add automated unit tests that cover the sidebar cookie persistence behavior: test the React.useEffect restore path by baking document.cookie with SIDEBAR_COOKIE_NAME=true/false and asserting _setOpen/open reflected, test the write path by calling the setOpen function (both with a boolean and with a functional updater) and assert document.cookie contains SIDEBAR_COOKIE_NAME=<value> and uses SIDEBAR_COOKIE_MAX_AGE and path=/, test error-tolerant paths by simulating exceptions when reading/writing document.cookie (e.g., throwing on get/set) and asserting no uncaught errors, and cover both controlled mode (when setOpenProp is provided) and uncontrolled mode (using _setOpen) to ensure the correct setter is invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/components/Navigation/Sidebar/AppSidebar.tsx`:
- Around line 117-132: The icon-only toggle Button in AppSidebar lacks an
accessible name; update the Button (the one using onClick={toggleSidebar} and
rendering <ChevronLeft>) to include a dynamic aria-label like aria-label={open ?
'Collapse sidebar' : 'Expand sidebar'} (or alternatively add an sr-only <span>
with the same dynamic text inside the Button) so screen readers announce the
action tied to the open state.
---
Outside diff comments:
In `@frontend/src/components/ui/sidebar.tsx`:
- Around line 73-103: Add automated unit tests that cover the sidebar cookie
persistence behavior: test the React.useEffect restore path by baking
document.cookie with SIDEBAR_COOKIE_NAME=true/false and asserting _setOpen/open
reflected, test the write path by calling the setOpen function (both with a
boolean and with a functional updater) and assert document.cookie contains
SIDEBAR_COOKIE_NAME=<value> and uses SIDEBAR_COOKIE_MAX_AGE and path=/, test
error-tolerant paths by simulating exceptions when reading/writing
document.cookie (e.g., throwing on get/set) and asserting no uncaught errors,
and cover both controlled mode (when setOpenProp is provided) and uncontrolled
mode (using _setOpen) to ensure the correct setter is invoked.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6775bc3e-236c-48ee-81fe-8a58adcfd072
📒 Files selected for processing (5)
frontend/index.htmlfrontend/src/components/Media/ChronologicalGallery.tsxfrontend/src/components/Navigation/Sidebar/AppSidebar.tsxfrontend/src/components/ui/sidebar.tsxfrontend/src/pages/Home/MyFav.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/components/Navigation/Sidebar/AppSidebar.tsx (1)
117-133: ⚡ Quick winConsider extracting the duplicate label text.
The label text
open ? 'Collapse sidebar' : 'Expand sidebar'is repeated on lines 121 and 132. While both serve different purposes (aria-label for screen readers, tooltip for sighted users), extracting this to a computed constant would improve maintainability and ensure consistency.♻️ Suggested refactor
<TooltipTrigger asChild> + {(() => { + const toggleLabel = open ? 'Collapse sidebar' : 'Expand sidebar'; + return ( <Button variant="ghost" size="icon" onClick={toggleSidebar} - aria-label={open ? 'Collapse sidebar' : 'Expand sidebar'} + aria-label={toggleLabel} className="hover:bg-accent h-8 w-8 transition-transform duration-300 ease-in-out" > <ChevronLeft className={`h-5 w-5 transition-transform duration-300 ease-in-out ${ !open ? 'rotate-180' : '' }`} /> </Button> + ); + })()} </TooltipTrigger> <TooltipContent side="right"> - <p>{open ? 'Collapse sidebar' : 'Expand sidebar'}</p> + <p>{open ? 'Collapse sidebar' : 'Expand sidebar'}</p> </TooltipContent>Or alternatively, compute it before the return statement:
const menuItems = [ { name: 'Home', path: `/${ROUTES.HOME}`, icon: Home }, // ... ]; + const toggleLabel = open ? 'Collapse sidebar' : 'Expand sidebar'; return ( <SidebarThen use
toggleLabelin both locations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/Navigation/Sidebar/AppSidebar.tsx` around lines 117 - 133, In AppSidebar, the string expression used twice for the toggle button label (used as the Button aria-label and in TooltipContent) should be extracted to a single computed constant to avoid duplication and ensure consistency; compute a const like toggleLabel = open ? 'Collapse sidebar' : 'Expand sidebar' (e.g., just before the return in the AppSidebar component) and then replace both occurrences (the Button aria-label and the TooltipContent paragraph) with that variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@frontend/src/components/Navigation/Sidebar/AppSidebar.tsx`:
- Around line 117-133: In AppSidebar, the string expression used twice for the
toggle button label (used as the Button aria-label and in TooltipContent) should
be extracted to a single computed constant to avoid duplication and ensure
consistency; compute a const like toggleLabel = open ? 'Collapse sidebar' :
'Expand sidebar' (e.g., just before the return in the AppSidebar component) and
then replace both occurrences (the Button aria-label and the TooltipContent
paragraph) with that variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b0cb425e-7cea-43ad-8cde-aad19cb47fd9
📒 Files selected for processing (1)
frontend/src/components/Navigation/Sidebar/AppSidebar.tsx
2 participants
Addressed Issues:
Fixes Issue
#942
Fixes Conflicted PR
#1116
Screenshots/Recordings:
NA
Additional Notes:
NA
AI Usage Disclosure:
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact. AI slop is strongly discouraged and may lead to banning and blocking. Do not spam our repos with AI slop.
Check one of the checkboxes below:
I have used the following AI models and tools: Claude
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
UI Improvements