Info panel doesn't close on outside click#1204
Info panel doesn't close on outside click#1204Samanvitha275 wants to merge 2 commits intoAOSSIE-Org:mainfrom
Conversation
WalkthroughThis pull request removes duplicate logger initialization in a Python utility file and enhances the MediaInfoPanel React component by adding outside-click-to-close functionality and expanding the UI with additional metadata sections (Date, Location, Tags, Position). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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: 2
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/Media/MediaInfoPanel.tsx (1)
123-141:⚠️ Potential issue | 🟡 MinorTreat
0latitude/longitude as valid coordinates.The current truthy checks hide valid coordinates when either value is
0(e.g., equator/prime meridian). Use!= nullchecks instead.Proposed fix
- if (currentImage?.metadata?.latitude && currentImage?.metadata?.longitude) { + if (currentImage?.metadata?.latitude != null && currentImage?.metadata?.longitude != null) { const { latitude, longitude } = currentImage.metadata; try { await open(`https://maps.google.com/?q=${latitude},${longitude}`); } catch (error) { console.error('Failed to open map URL:', error); } }- {currentImage?.metadata?.latitude && - currentImage?.metadata?.longitude ? ( + {currentImage?.metadata?.latitude != null && + currentImage?.metadata?.longitude != null ? (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/Media/MediaInfoPanel.tsx` around lines 123 - 141, The location visibility check currently treats 0 coordinates as falsy; update the conditional that checks currentImage?.metadata?.latitude and longitude in MediaInfoPanel (referencing currentImage, metadata, latitude, longitude, and handleLocationClick) to use null/undefined-safe checks (e.g., !== null and !== undefined or != null) so that 0 is considered valid — keep the button rendering and onClick (handleLocationClick) unchanged but change the boolean expression to test for not-null rather than truthiness.
🧹 Nitpick comments (1)
frontend/src/components/Media/MediaInfoPanel.tsx (1)
99-170: Section comments are mostly redundant and can be removed.Inline comments like
/* Name */,/* Date */, etc. mirror the visible labels and add noise without clarifying logic.As per coding guidelines, "Point out redundant obvious comments that do not add clarity to the code."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/Media/MediaInfoPanel.tsx` around lines 99 - 170, Remove the redundant inline section comments (e.g. {/* Name */}, {/* Date */}, {/* Location */}, {/* Tags */}, {/* Position */}) in MediaInfoPanel.tsx so the JSX reads cleaner; keep the existing elements and labels (the <p> elements that render Name, Date, Location, Tags and the button using handleLocationClick, and the usages of getImageName(), getFormattedDate(), and currentImage) unchanged — simply delete those comment lines around each block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/Media/MediaInfoPanel.tsx`:
- Around line 33-44: Add automated React Testing Library tests for
MediaInfoPanel that exercise the useEffect outside-click logic: write one test
that renders <MediaInfoPanel ... show={true} onClose={mockFn}>, simulates a
click inside the panel (target within panelRef) and asserts onClose is not
called, and another test that simulates a click outside the panel and asserts
onClose is called once; reference the component MediaInfoPanel, the
effect/handler handleClickOutside, panelRef and the onClose prop when locating
where to exercise behavior, and use fireEvent or userEvent to simulate
'mousedown' events to match the document listener.
- Line 186: The onClick handler in MediaInfoPanel currently does nothing
(onClick={(e) => e.preventDefault()}); restore the original action by calling
the component's real opener method instead of just preventDefault. Replace that
no-op with the existing handler used to open the original file (e.g., call
handleOpenOriginal or openOriginal or props.onOpenOriginal depending on which
symbol exists in MediaInfoPanel) and keep preventDefault only if the original
implementation did so; ensure the onClick invokes the actual
open/download/navigation logic so the "Open Original File" button performs its
intended action.
---
Outside diff comments:
In `@frontend/src/components/Media/MediaInfoPanel.tsx`:
- Around line 123-141: The location visibility check currently treats 0
coordinates as falsy; update the conditional that checks
currentImage?.metadata?.latitude and longitude in MediaInfoPanel (referencing
currentImage, metadata, latitude, longitude, and handleLocationClick) to use
null/undefined-safe checks (e.g., !== null and !== undefined or != null) so that
0 is considered valid — keep the button rendering and onClick
(handleLocationClick) unchanged but change the boolean expression to test for
not-null rather than truthiness.
---
Nitpick comments:
In `@frontend/src/components/Media/MediaInfoPanel.tsx`:
- Around line 99-170: Remove the redundant inline section comments (e.g. {/*
Name */}, {/* Date */}, {/* Location */}, {/* Tags */}, {/* Position */}) in
MediaInfoPanel.tsx so the JSX reads cleaner; keep the existing elements and
labels (the <p> elements that render Name, Date, Location, Tags and the button
using handleLocationClick, and the usages of getImageName(), getFormattedDate(),
and currentImage) unchanged — simply delete those comment lines around each
block.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/utils/images.pyfrontend/src/components/Media/MediaInfoPanel.tsx
💤 Files with no reviewable changes (1)
- backend/app/utils/images.py
| useEffect(() => { | ||
| function handleClickOutside(event: MouseEvent) { | ||
| if (!show) return; | ||
|
|
||
| if (panelRef.current && !panelRef.current.contains(event.target as Node)) { | ||
| onClose(); | ||
| } | ||
| } | ||
|
|
||
| document.addEventListener('mousedown', handleClickOutside); | ||
| return () => document.removeEventListener('mousedown', handleClickOutside); | ||
| }, [show, onClose]); |
There was a problem hiding this comment.
Add automated tests for outside-click close behavior.
This is the core bug fix path, but no test coverage is shown for clicking outside vs. clicking inside panel boundaries. Please add an automated UI test for both cases.
I can draft a React Testing Library test case for this if you want.
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 the current code and only fix it if needed.
In `@frontend/src/components/Media/MediaInfoPanel.tsx` around lines 33 - 44, Add
automated React Testing Library tests for MediaInfoPanel that exercise the
useEffect outside-click logic: write one test that renders <MediaInfoPanel ...
show={true} onClose={mockFn}>, simulates a click inside the panel (target within
panelRef) and asserts onClose is not called, and another test that simulates a
click outside the panel and asserts onClose is called once; reference the
component MediaInfoPanel, the effect/handler handleClickOutside, panelRef and
the onClose prop when locating where to exercise behavior, and use fireEvent or
userEvent to simulate 'mousedown' events to match the document listener.
| e.preventDefault(); | ||
| // Button disabled - does nothing | ||
| }} | ||
| onClick={(e) => e.preventDefault()} |
There was a problem hiding this comment.
Restore the Open Original File action (current handler is a no-op).
The button now only calls preventDefault(), so it no longer performs any user action. That is a functional regression and conflicts with the stated behavior that button interactions remain unchanged.
Proposed fix
- onClick={(e) => e.preventDefault()}
+ onClick={async () => {
+ if (!currentImage?.path) return;
+ try {
+ await open(currentImage.path);
+ } catch (error) {
+ console.error('Failed to open original file:', error);
+ }
+ }}As per coding guidelines, "Confirm that the code meets the project's requirements and objectives."
📝 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.
| onClick={(e) => e.preventDefault()} | |
| onClick={async () => { | |
| if (!currentImage?.path) return; | |
| try { | |
| await open(currentImage.path); | |
| } catch (error) { | |
| console.error('Failed to open original file:', error); | |
| } | |
| }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/Media/MediaInfoPanel.tsx` at line 186, The onClick
handler in MediaInfoPanel currently does nothing (onClick={(e) =>
e.preventDefault()}); restore the original action by calling the component's
real opener method instead of just preventDefault. Replace that no-op with the
existing handler used to open the original file (e.g., call handleOpenOriginal
or openOriginal or props.onOpenOriginal depending on which symbol exists in
MediaInfoPanel) and keep preventDefault only if the original implementation did
so; ensure the onClick invokes the actual open/download/navigation logic so the
"Open Original File" button performs its intended action.
|
Hi maintainers, |
1 participant
Fix: Close Media Info Panel on Outside Click
What
Fixes a UX bug where the media info panel remains open even after clicking on empty space outside the panel.
Why
Current behavior forces users to explicitly click the close (❌) button, which is inconsistent with common modal / overlay UX patterns and degrades usability.
How
Added outside-click detection to the MediaInfoPanel wrapper.
When a user clicks anywhere outside the info panel, onClose() is triggered.
Prevented event propagation inside the panel to avoid accidental closes.
No changes to existing keyboard or button behavior.
Result
Clicking on empty space now properly closes the info panel.
UX is more intuitive and aligned with user expectations.
Scope
Frontend only
No breaking changes
No dependency updates
Related Issue
Closes #1018
Summary by CodeRabbit
New Features
Bug Fixes