Add smooth exit animation to Image Details Panel #980
Conversation
📝 WalkthroughWalkthroughThe PR adds smooth exit animation to the Media Info Panel and introduces a reusable ScrollToTopButton component. The panel now animates out over 300ms before unmounting, improving visual consistency. The new ScrollToTopButton tracks scroll position and smoothly scrolls containers to the top when clicked. Both are integrated into the Home page. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 2
🤖 Fix all issues with AI agents
In @frontend/package.json:
- Line 91: Remove the unused dependency "baseline-browser-mapping" from
package.json by deleting its entry in the dependencies/devDependencies section,
then regenerate the lockfile (run npm install or yarn install) to update
package-lock.json/yarn.lock and ensure no code imports it; finally run the
test/build to confirm nothing breaks and commit the package.json and updated
lockfile changes.
In @frontend/src/components/Media/MediaInfoPanel.tsx:
- Around line 29-35: The effect watching show only sets isVisible true and never
responds to show becoming false, which can leave the panel stuck open; update
the useEffect that references show to setIsVisible(show) (or when preserving
exit animation, set isVisible true on show true and on show false start the exit
animation by calling setIsVisible(false) after the animation timeout) so
isVisible always follows the parent-driven show state; adjust logic around the
rendering guard (the condition that returns null when both show and isVisible
are false) to remain correct with the new effect and keep references to
isVisible, setIsVisible, and the useEffect callback.
🧹 Nitpick comments (1)
frontend/src/components/Media/MediaInfoPanel.tsx (1)
37-40: Consider cleanup for the timeout.The
setTimeoutcould potentially fire after the component unmounts if the parent immediately setsshow={false}and unmounts the component. While unlikely to cause issues in practice, cleaning up the timeout is a defensive best practice.🔎 Proposed cleanup
+ useEffect(() => { + // Cleanup any pending timeout on unmount + return () => { + // Store timeout ID in a ref if needed for cleanup + }; + }, []); + const handleClose = () => { setIsVisible(false); - setTimeout(onClose, 300); // must match animation duration + const timer = setTimeout(onClose, 300); // must match animation duration + // If using cleanup, store timer in a ref here };Alternatively, use a ref to store and clear the timer:
+ const timeoutRef = useRef<NodeJS.Timeout | null>(null); + + useEffect(() => { + return () => { + if (timeoutRef.current) { + clearTimeout(timeoutRef.current); + } + }; + }, []); + const handleClose = () => { setIsVisible(false); + timeoutRef.current = setTimeout(onClose, 300); - setTimeout(onClose, 300); // must match animation duration };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
frontend/package.jsonfrontend/src/components/Media/MediaInfoPanel.tsx
🔇 Additional comments (2)
frontend/src/components/Media/MediaInfoPanel.tsx (2)
78-96: LGTM! Clean animation implementation.The exit animation implementation is well-executed:
- The 300ms
duration-300class matches the timeout inhandleClose(line 39)- Smooth transition using opacity and translate transforms
- Proper use of
isVisiblestate to control animation classes- Close button correctly triggers
handleCloseinstead of directly callingonCloseThis creates a cohesive slide-and-fade effect that mirrors the entry animation pattern.
131-132: Note: Location logic unchanged.The AI-generated summary states that the location logic was modified to "show the map button only if longitude exists," but the actual code still requires both
latitudeANDlongitude(line 131-132). The logic appears correct as-is—both coordinates are needed for a valid map location—but this discrepancy between the summary and code should be noted.
| const [isVisible, setIsVisible] = useState(show); | ||
|
|
||
| useEffect(() => { | ||
| if (show) { | ||
| setIsVisible(true); | ||
| } | ||
| }, [show]); |
There was a problem hiding this comment.
Critical: Handle show becoming false to prevent stuck panel.
The current effect only sets isVisible to true when show becomes true, but doesn't handle the case when show becomes false. This creates a critical desynchronization bug:
Scenario: If the parent component sets show={false} directly (without triggering handleClose), the panel will become stuck visible because:
showbecomesfalse(from parent)isVisibleremainstrue(no effect updates it)- The rendering guard on line 74 still renders the component (only returns
nullwhen BOTH are false) - The panel displays at full opacity indefinitely (line 84 uses
isVisibleto control visibility classes)
Impact: The panel cannot be closed if the parent updates show directly, breaking the close functionality in certain usage patterns.
🔎 Proposed fix
useEffect(() => {
if (show) {
setIsVisible(true);
+ } else {
+ // Animate out when parent sets show to false
+ setIsVisible(false);
}
}, [show]);Alternatively, if you want to preserve the exit animation when the parent closes directly:
useEffect(() => {
if (show) {
setIsVisible(true);
+ } else {
+ // Trigger exit animation, but let parent control final unmount
+ const timer = setTimeout(() => setIsVisible(false), 0);
+ return () => clearTimeout(timer);
}
}, [show]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [isVisible, setIsVisible] = useState(show); | |
| useEffect(() => { | |
| if (show) { | |
| setIsVisible(true); | |
| } | |
| }, [show]); | |
| const [isVisible, setIsVisible] = useState(show); | |
| useEffect(() => { | |
| if (show) { | |
| setIsVisible(true); | |
| } else { | |
| // Animate out when parent sets show to false | |
| setIsVisible(false); | |
| } | |
| }, [show]); |
🤖 Prompt for AI Agents
In @frontend/src/components/Media/MediaInfoPanel.tsx around lines 29 - 35, The
effect watching show only sets isVisible true and never responds to show
becoming false, which can leave the panel stuck open; update the useEffect that
references show to setIsVisible(show) (or when preserving exit animation, set
isVisible true on show true and on show false start the exit animation by
calling setIsVisible(false) after the animation timeout) so isVisible always
follows the parent-driven show state; adjust logic around the rendering guard
(the condition that returns null when both show and isVisible are false) to
remain correct with the new effect and keep references to isVisible,
setIsVisible, and the useEffect callback.
|
@sayyadshahid Hey, Just a heads up, this issue (#972) was already assigned to me, and I’ve opened a PR addressing it here: #978. Since the issue was already assigned, it’s best to avoid parallel PRs and follow the project’s contribution guidelines to prevent duplicate work. |
removed this dependency
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @frontend/src/components/ScrollToTopButton/ScrollToTopButton.tsx:
- Around line 14-28: The effect that attaches the scroll listener uses
scrollContainerRef but doesn't depend on its .current, so replacing
scrollContainerRef?.current into the dependency array will ensure the effect
reruns when the actual DOM container changes; update the useEffect dependency
list to include scrollContainerRef?.current and threshold so the listener
attached by handleScroll (which reads scroller = scrollContainerRef?.current ||
window and calls setVisible based on scrollTop) is cleaned up and re-attached
for the new element.
- Around line 46-47: In ScrollToTopButton's className (the string containing
"fixed top-35 right-15") replace the non-standard Tailwind spacing classes with
valid Tailwind spacing tokens (e.g., use bottom-20 right-6 or top-9 right-4
depending on desired placement) so the positioning utilities are generated;
update the className in the ScrollToTopButton component string to a combination
of standard classes like "fixed bottom-20 right-6 z-50" (or your chosen valid
alternatives).
🧹 Nitpick comments (1)
frontend/src/components/ScrollToTopButton/ScrollToTopButton.tsx (1)
30-38: Inconsistent scroller reference handling.The
scrollToTopfunction checksscrollContainerRef?.currentseparately and falls back towindow, but theuseEffectusesscrollContainerRef?.current || windowinline. This inconsistency makes the code harder to reason about.Consider extracting the scroller resolution logic into a helper function or using the same pattern in both places.
♻️ Refactor for consistency
+ const getScroller = () => scrollContainerRef?.current || window; + useEffect(() => { - const scroller = scrollContainerRef?.current || window; + const scroller = getScroller(); const handleScroll = () => { const scrollTop = scroller instanceof Window ? window.scrollY : scroller.scrollTop; setVisible(scrollTop > threshold); }; scroller.addEventListener('scroll', handleScroll); return () => scroller.removeEventListener('scroll', handleScroll); }, [scrollContainerRef, threshold]); const scrollToTop = () => { - const scroller = scrollContainerRef?.current; - - if (scroller) { - scroller.scrollTo({ top: 0, behavior: 'smooth' }); - } else { - window.scrollTo({ top: 0, behavior: 'smooth' }); - } + getScroller().scrollTo({ top: 0, behavior: 'smooth' }); };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
frontend/src/components/ScrollToTopButton/ScrollToTopButton.tsxfrontend/src/pages/Home/Home.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/pages/Home/Home.tsx (1)
frontend/src/components/ScrollToTopButton/ScrollToTopButton.tsx (1)
ScrollToTopButton(8-57)
🔇 Additional comments (3)
frontend/src/components/ScrollToTopButton/ScrollToTopButton.tsx (1)
1-56: PR scope inconsistency: ScrollToTopButton not mentioned in objectives.The PR objectives state this PR adds "smooth exit animation to Image Details Panel" and fixes issue #972, but this file introduces an entirely new ScrollToTopButton component. The AI summary mentions both features, suggesting files related to the Image Details Panel exit animation may not be included in this review batch.
Please clarify:
- Is the ScrollToTopButton part of PR #980, or was it accidentally included?
- Are there additional files containing the Image Details Panel exit animation that should be reviewed?
frontend/src/pages/Home/Home.tsx (2)
16-16: ScrollToTopButton integration looks correct, but scope concerns remain.The integration of ScrollToTopButton is technically sound:
- The import path is correct
scrollableRefis properly typed asReact.RefObject<HTMLDivElement>- The prop passing matches the expected interface
However, this addition doesn't align with the stated PR objectives (exit animation for Image Details Panel). Please confirm this feature is intentional for this PR.
Also applies to: 83-83
52-52: Minor formatting changes.The whitespace adjustments improve code readability and are consistent with the file's existing style.
Also applies to: 82-85
| useEffect(() => { | ||
| const scroller = scrollContainerRef?.current || window; | ||
|
|
||
| const handleScroll = () => { | ||
| const scrollTop = | ||
| scroller instanceof Window | ||
| ? window.scrollY | ||
| : scroller.scrollTop; | ||
|
|
||
| setVisible(scrollTop > threshold); | ||
| }; | ||
|
|
||
| scroller.addEventListener('scroll', handleScroll); | ||
| return () => scroller.removeEventListener('scroll', handleScroll); | ||
| }, [scrollContainerRef, threshold]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat frontend/tsconfig.json | jq '.compilerOptions | {strict, strictNullChecks, strictFunctionTypes}'Repository: AOSSIE-Org/PictoPy
Length of output: 119
🏁 Script executed:
cat -n frontend/src/components/ScrollToTopButton/ScrollToTopButton.tsxRepository: AOSSIE-Org/PictoPy
Length of output: 1821
🏁 Script executed:
cat frontend/tsconfig.jsonRepository: AOSSIE-Org/PictoPy
Length of output: 804
🏁 Script executed:
# Check the actual types being used - both Window and HTMLElement should inherit from EventTarget
# which has addEventListener and removeEventListener
rg -A 5 "addEventListener" frontend/src/components/ScrollToTopButton/ScrollToTopButton.tsxRepository: AOSSIE-Org/PictoPy
Length of output: 307
Fix dependency array to track actual ref value changes.
The dependency array should include scrollContainerRef?.current instead of scrollContainerRef. Since scrollContainerRef is a ref object with stable identity, changes to its .current property won't trigger a re-run of the effect. If the container element changes after mount, the scroll event listener won't be updated.
The type safety concern about incompatible addEventListener signatures is not applicable here—both Window and HTMLElement implement the EventTarget interface with compatible method signatures. With "strict": true enabled in your tsconfig, TypeScript will catch actual type errors.
Update line 28 to: }, [scrollContainerRef?.current, threshold]);
🤖 Prompt for AI Agents
In @frontend/src/components/ScrollToTopButton/ScrollToTopButton.tsx around lines
14 - 28, The effect that attaches the scroll listener uses scrollContainerRef
but doesn't depend on its .current, so replacing scrollContainerRef?.current
into the dependency array will ensure the effect reruns when the actual DOM
container changes; update the useEffect dependency list to include
scrollContainerRef?.current and threshold so the listener attached by
handleScroll (which reads scroller = scrollContainerRef?.current || window and
calls setVisible based on scrollTop) is cleaned up and re-attached for the new
element.
| className=" | ||
| fixed top-35 right-15 z-50 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine Tailwind configuration
find . -type f \( -name "tailwind.config.*" -o -name "tailwind.config.js" -o -name "tailwind.config.ts" \) 2>/dev/nullRepository: AOSSIE-Org/PictoPy
Length of output: 121
🏁 Script executed:
# Look at the ScrollToTopButton component file
find . -path "*/frontend/src/components/ScrollToTopButton/ScrollToTopButton.tsx" -type fRepository: AOSSIE-Org/PictoPy
Length of output: 127
🏁 Script executed:
# Search for custom spacing definitions across config files
find . -type f \( -name "*.config.*" -o -name "postcss.config.*" \) | head -20Repository: AOSSIE-Org/PictoPy
Length of output: 321
🏁 Script executed:
# Read the ScrollToTopButton component
cat -n ./frontend/src/components/ScrollToTopButton/ScrollToTopButton.tsxRepository: AOSSIE-Org/PictoPy
Length of output: 1821
🏁 Script executed:
# Search for Tailwind config in frontend directory
find ./frontend -name "tailwind.config.*" -o -name "*tailwind*" -type f 2>/dev/null | grep -v node_modulesRepository: AOSSIE-Org/PictoPy
Length of output: 44
🏁 Script executed:
# Look for any CSS configuration or custom Tailwind setup in frontend
find ./frontend -type f \( -name "*.config.*" -o -name "vite.config.*" \) | xargs ls -laRepository: AOSSIE-Org/PictoPy
Length of output: 285
🏁 Script executed:
# Check if there's a root-level tailwind config
find . -maxdepth 2 -name "tailwind.config.*" -type fRepository: AOSSIE-Org/PictoPy
Length of output: 121
🏁 Script executed:
# Read the frontend vite.config to see if Tailwind is configured there
cat ./frontend/vite.config.tsRepository: AOSSIE-Org/PictoPy
Length of output: 869
🏁 Script executed:
# Check frontend package.json for Tailwind setup
cat ./frontend/package.json | grep -A 5 -B 5 tailwind || echo "Not found in package.json"Repository: AOSSIE-Org/PictoPy
Length of output: 1169
🏁 Script executed:
# Read one of the found tailwind configs to understand the structure
cat ./landing-page/tailwind.config.jsRepository: AOSSIE-Org/PictoPy
Length of output: 1477
🏁 Script executed:
# Search the entire frontend directory for any tailwind config file
find ./frontend -type f -name "tailwind.config.*"Repository: AOSSIE-Org/PictoPy
Length of output: 44
🏁 Script executed:
# Search for top-35 or right-15 definitions in the codebase
rg "top-35|right-15" --type-list | head -5
rg "top-35|right-15"Repository: AOSSIE-Org/PictoPy
Length of output: 567
🏁 Script executed:
# Check if there's a CSS file defining these classes
find ./frontend -type f \( -name "*.css" -o -name "*.scss" \) | head -20Repository: AOSSIE-Org/PictoPy
Length of output: 84
🏁 Script executed:
# Check the CSS file to see if custom classes are defined there
cat ./frontend/src/App.cssRepository: AOSSIE-Org/PictoPy
Length of output: 7941
🏁 Script executed:
# Also check for any tailwind directives or custom CSS in the main index/entry files
find ./frontend/src -maxdepth 1 -type f \( -name "*.css" -o -name "main.*" -o -name "index.*" \)Repository: AOSSIE-Org/PictoPy
Length of output: 108
Replace non-standard Tailwind spacing classes with valid alternatives.
The classes top-35 and right-15 are not standard Tailwind spacing values and are not defined in your Tailwind configuration. These classes will not be generated and the button positioning will fail silently.
Use standard Tailwind spacing increments (0, 1, 2, 3, 4, 6, 8, 10, 12, 14, 16, 20, 24, 28, 32, 36, 40, 44, 48, 52, 56, 60, 64, 72, 80, 96) instead.
🎨 Example fix
className="
- fixed top-35 right-15 z-50
+ fixed bottom-20 right-6 z-50
rounded-full bg-primary p-3
text-white shadow-lg
transition-all duration-300
hover:scale-110 hover:shadow-xl
"(Using bottom-20 right-6 for typical scroll-to-top button placement; adjust as needed.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| className=" | |
| fixed top-35 right-15 z-50 | |
| className=" | |
| fixed bottom-20 right-6 z-50 | |
| rounded-full bg-primary p-3 | |
| text-white shadow-lg | |
| transition-all duration-300 | |
| hover:scale-110 hover:shadow-xl | |
| " |
🤖 Prompt for AI Agents
In @frontend/src/components/ScrollToTopButton/ScrollToTopButton.tsx around lines
46 - 47, In ScrollToTopButton's className (the string containing "fixed top-35
right-15") replace the non-standard Tailwind spacing classes with valid Tailwind
spacing tokens (e.g., use bottom-20 right-6 or top-9 right-4 depending on
desired placement) so the positioning utilities are generated; update the
className in the ScrollToTopButton component string to a combination of standard
classes like "fixed bottom-20 right-6 z-50" (or your chosen valid alternatives).
2 participants
This PR adds a smooth exit animation to the Image Details panel so that it closes consistently instead of disappearing abruptly. The open and close interactions now feel visually cohesive and polished.
Before
532530563-fc7e250c-18de-4cc3-b43a-fe77c54f68d0.mp4
After
Screencast from 2026-01-07 09-32-11.webm
Changes
Scope
Fixes
Additional Notes
This implementation follows existing animation patterns already used in the codebase and does not introduce any new dependencies. The change improves user experience without affecting functionality or performance.
Summary by CodeRabbit
Release Notes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.