Experimented gallery lightbox feature in ActivityPub articles#22642
Experimented gallery lightbox feature in ActivityPub articles#22642minimaluminium wants to merge 1 commit intomainfrom
Conversation
WalkthroughThe changes enhance the Possibly Related PRs
Suggested Reviewers
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
apps/admin-x-activitypub/src/components/feed/ArticleModal.tsxOops! Something went wrong! :( ESLint: 8.44.0 ESLint couldn't find the plugin "eslint-plugin-react-hooks". (The package "eslint-plugin-react-hooks" was not found when loaded as a Node module from the directory "/apps/admin-x-activitypub".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-react-hooks" was referenced from the config file in "apps/admin-x-activitypub/.eslintrc.cjs". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (2)
apps/admin-x-activitypub/src/components/feed/ArticleModal.tsx (2)
122-137: Missing Escape key handling for closing the lightbox.While left and right arrow keys are implemented, users typically expect to close lightboxes using the Escape key.
Add Escape key support:
useEffect(() => { const handleKeyDown = (e: KeyboardEvent) => { if (!dialogOpen) { return; } if (e.key === 'ArrowLeft') { goToPrevious(); } else if (e.key === 'ArrowRight') { goToNext(); + } else if (e.key === 'Escape') { + setDialogOpen(false); } }; window.addEventListener('keydown', handleKeyDown); return () => window.removeEventListener('keydown', handleKeyDown); }, [dialogOpen, goToNext, goToPrevious, images]);
408-434: Add navigation buttons to the lightbox.Currently, the lightbox only supports keyboard navigation. Consider adding visible navigation buttons for better usability, especially on touch devices.
<Dialog open={dialogOpen} onOpenChange={setDialogOpen}> <DialogContent className="max-h-[90vh] max-w-[90vw] overflow-hidden bg-black/90 p-0"> {currentImage && ( <div className="relative flex h-full items-center justify-center"> + {/* Previous button */} + {images.length > 1 && ( + <button + className="absolute left-4 top-1/2 flex h-10 w-10 -translate-y-1/2 items-center justify-center rounded-full bg-black/50 text-white hover:bg-black/70" + onClick={(e) => { + e.stopPropagation(); + goToPrevious(); + }} + > + <Icon name="arrow-left" /> + </button> + )} <div className="flex flex-col items-center"> <img alt={currentImage.alt || ''} className="max-h-[80vh] max-w-full object-contain" src={currentImage.src} /> {/* Caption and counter */} <div className="w-full p-4 text-center"> {currentImage.alt && ( <p className="mb-2 text-sm text-white">{currentImage.alt}</p> )} {images.length > 1 && ( <p className="text-xs text-white/70"> {(currentIndex ?? 0) + 1} / {images.length} </p> )} </div> </div> + {/* Next button */} + {images.length > 1 && ( + <button + className="absolute right-4 top-1/2 flex h-10 w-10 -translate-y-1/2 items-center justify-center rounded-full bg-black/50 text-white hover:bg-black/70" + onClick={(e) => { + e.stopPropagation(); + goToNext(); + }} + > + <Icon name="arrow-right" /> + </button> + )} + {/* Close button */} + <button + className="absolute right-4 top-4 flex h-10 w-10 items-center justify-center rounded-full bg-black/50 text-white hover:bg-black/70" + onClick={() => setDialogOpen(false)} + > + <Icon name="close" /> + </button> </div> )} </DialogContent> </Dialog>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/admin-x-activitypub/src/components/feed/ArticleModal.tsx(5 hunks)
🔇 Additional comments (5)
apps/admin-x-activitypub/src/components/feed/ArticleModal.tsx (5)
28-29: Good import addition for lightbox functionality.The Dialog components from @tryghost/shade are properly imported for the new gallery lightbox feature.
50-55: Good interface definition for gallery images.The GalleryImage interface is well-structured with appropriate types for image properties.
92-94: State hooks properly set up for the gallery feature.The useState hooks for managing gallery state (images array, current index, and dialog visibility) are correctly implemented.
113-119: Navigation functions well implemented.The
goToPreviousandgoToNextfunctions correctly handle image navigation with proper boundary handling and state updates.
139-139: Good implementation of current image access.The current image is correctly determined based on the currentIndex with proper type casting.
| useEffect(() => { | ||
| // Listen for messages from the iframe | ||
| const handleMessage = (event: MessageEvent) => { | ||
| // You might want to check event.origin for security | ||
|
|
||
| if (event.data.type === 'REGISTER_IMAGES') { | ||
| setImages(event.data.payload); | ||
| } else if (event.data.type === 'OPEN_LIGHTBOX') { | ||
| setCurrentIndex(event.data.payload.index); | ||
| setDialogOpen(true); | ||
| } | ||
| }; | ||
|
|
||
| window.addEventListener('message', handleMessage); | ||
| return () => window.removeEventListener('message', handleMessage); | ||
| }, []); |
There was a problem hiding this comment.
Missing origin validation in message handler.
The comment on line 99 indicates that origin checking should be implemented for security, but this hasn't been done.
Add validation to prevent potential cross-site scripting attacks:
const handleMessage = (event: MessageEvent) => {
- // You might want to check event.origin for security
+ // Only accept messages from our own domain
+ if (event.origin !== window.location.origin) {
+ return;
+ }
if (event.data.type === 'REGISTER_IMAGES') {📝 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.
| useEffect(() => { | |
| // Listen for messages from the iframe | |
| const handleMessage = (event: MessageEvent) => { | |
| // You might want to check event.origin for security | |
| if (event.data.type === 'REGISTER_IMAGES') { | |
| setImages(event.data.payload); | |
| } else if (event.data.type === 'OPEN_LIGHTBOX') { | |
| setCurrentIndex(event.data.payload.index); | |
| setDialogOpen(true); | |
| } | |
| }; | |
| window.addEventListener('message', handleMessage); | |
| return () => window.removeEventListener('message', handleMessage); | |
| }, []); | |
| useEffect(() => { | |
| // Listen for messages from the iframe | |
| const handleMessage = (event: MessageEvent) => { | |
| // Only accept messages from our own domain | |
| if (event.origin !== window.location.origin) { | |
| return; | |
| } | |
| if (event.data.type === 'REGISTER_IMAGES') { | |
| setImages(event.data.payload); | |
| } else if (event.data.type === 'OPEN_LIGHTBOX') { | |
| setCurrentIndex(event.data.payload.index); | |
| setDialogOpen(true); | |
| } | |
| }; | |
| window.addEventListener('message', handleMessage); | |
| return () => window.removeEventListener('message', handleMessage); | |
| }, []); |
| <script> | ||
| // When the iframe loads, register all images | ||
| function registerImages() { | ||
| const allImages = Array.from(document.querySelectorAll('.kg-gallery-card img')).map((img, index) => ({ | ||
| src: img.src, | ||
| alt: img.alt, | ||
| index: index | ||
| })); | ||
|
|
||
| window.parent.postMessage({ | ||
| type: 'REGISTER_IMAGES', | ||
| payload: allImages | ||
| }, '*'); | ||
| } | ||
|
|
||
| // Call this when the iframe content loads | ||
| window.addEventListener('load', registerImages); | ||
|
|
||
| // Add click listeners to your images | ||
| document.querySelectorAll('.kg-gallery-card img').forEach((img, index) => { | ||
| img.addEventListener('click', (e) => { | ||
| e.preventDefault(); | ||
| window.parent.postMessage({ | ||
| type: 'OPEN_LIGHTBOX', | ||
| payload: { index: index } | ||
| }, '*'); | ||
| }); | ||
| }); | ||
| </script> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider more robust image registration approach.
The current implementation has a few limitations:
- It only targets
.kg-gallery-card imgelements - It may miss images loaded after the script runs
- Event listeners might not be cleaned up properly
Consider a more robust implementation:
<script>
+ const registeredImages = [];
+ const registeredImageNodes = new WeakMap();
// When the iframe loads, register all images
function registerImages() {
- const allImages = Array.from(document.querySelectorAll('.kg-gallery-card img')).map((img, index) => ({
+ // Target all gallery images with a more inclusive selector
+ const allImages = Array.from(document.querySelectorAll('.kg-gallery-card img, .kg-image-card img, .kg-gallery-image img')).map((img, index) => ({
src: img.src,
alt: img.alt,
+ width: img.naturalWidth || 0,
+ height: img.naturalHeight || 0,
index: index
}));
+
+ // Store reference to registered images
+ registeredImages.length = 0;
+ registeredImages.push(...allImages);
window.parent.postMessage({
type: 'REGISTER_IMAGES',
payload: allImages
}, '*');
+
+ // Set up click handlers after registration
+ setupImageClickHandlers();
}
+ // Function to set up click handlers
+ function setupImageClickHandlers() {
+ const imageElements = document.querySelectorAll('.kg-gallery-card img, .kg-image-card img, .kg-gallery-image img');
+
+ imageElements.forEach((img, index) => {
+ // Remove previous handler if exists
+ if (registeredImageNodes.has(img)) {
+ img.removeEventListener('click', registeredImageNodes.get(img));
+ }
+
+ const clickHandler = (e) => {
+ e.preventDefault();
+ window.parent.postMessage({
+ type: 'OPEN_LIGHTBOX',
+ payload: { index: index }
+ }, '*');
+ };
+
+ img.addEventListener('click', clickHandler);
+ registeredImageNodes.set(img, clickHandler);
+ img.style.cursor = 'pointer';
+ });
+ }
// Call this when the iframe content loads
window.addEventListener('load', registerImages);
- // Add click listeners to your images
- document.querySelectorAll('.kg-gallery-card img').forEach((img, index) => {
- img.addEventListener('click', (e) => {
- e.preventDefault();
- window.parent.postMessage({
- type: 'OPEN_LIGHTBOX',
- payload: { index: index }
- }, '*');
- });
- });
+ // Setup a MutationObserver to detect dynamically added images
+ const observer = new MutationObserver(() => {
+ registerImages();
+ });
+
+ // Start observing
+ observer.observe(document.body, {
+ childList: true,
+ subtree: true
+ });
</script>📝 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.
| <script> | |
| // When the iframe loads, register all images | |
| function registerImages() { | |
| const allImages = Array.from(document.querySelectorAll('.kg-gallery-card img')).map((img, index) => ({ | |
| src: img.src, | |
| alt: img.alt, | |
| index: index | |
| })); | |
| window.parent.postMessage({ | |
| type: 'REGISTER_IMAGES', | |
| payload: allImages | |
| }, '*'); | |
| } | |
| // Call this when the iframe content loads | |
| window.addEventListener('load', registerImages); | |
| // Add click listeners to your images | |
| document.querySelectorAll('.kg-gallery-card img').forEach((img, index) => { | |
| img.addEventListener('click', (e) => { | |
| e.preventDefault(); | |
| window.parent.postMessage({ | |
| type: 'OPEN_LIGHTBOX', | |
| payload: { index: index } | |
| }, '*'); | |
| }); | |
| }); | |
| </script> | |
| <script> | |
| const registeredImages = []; | |
| const registeredImageNodes = new WeakMap(); | |
| // When the iframe loads, register all images | |
| function registerImages() { | |
| // Target all gallery images with a more inclusive selector | |
| const allImages = Array.from( | |
| document.querySelectorAll( | |
| '.kg-gallery-card img, .kg-image-card img, .kg-gallery-image img' | |
| ) | |
| ).map((img, index) => ({ | |
| src: img.src, | |
| alt: img.alt, | |
| width: img.naturalWidth || 0, | |
| height: img.naturalHeight || 0, | |
| index: index | |
| })); | |
| // Store reference to registered images | |
| registeredImages.length = 0; | |
| registeredImages.push(...allImages); | |
| window.parent.postMessage( | |
| { | |
| type: 'REGISTER_IMAGES', | |
| payload: allImages | |
| }, | |
| '*' | |
| ); | |
| // Set up click handlers after registration | |
| setupImageClickHandlers(); | |
| } | |
| // Function to set up click handlers | |
| function setupImageClickHandlers() { | |
| const imageElements = document.querySelectorAll( | |
| '.kg-gallery-card img, .kg-image-card img, .kg-gallery-image img' | |
| ); | |
| imageElements.forEach((img, index) => { | |
| // Remove previous handler if exists | |
| if (registeredImageNodes.has(img)) { | |
| img.removeEventListener('click', registeredImageNodes.get(img)); | |
| } | |
| const clickHandler = (e) => { | |
| e.preventDefault(); | |
| window.parent.postMessage( | |
| { | |
| type: 'OPEN_LIGHTBOX', | |
| payload: { index: index } | |
| }, | |
| '*' | |
| ); | |
| }; | |
| img.addEventListener('click', clickHandler); | |
| registeredImageNodes.set(img, clickHandler); | |
| img.style.cursor = 'pointer'; | |
| }); | |
| } | |
| // Call this when the iframe content loads | |
| window.addEventListener('load', registerImages); | |
| // Setup a MutationObserver to detect dynamically added images | |
| const observer = new MutationObserver(() => { | |
| registerImages(); | |
| }); | |
| // Start observing | |
| observer.observe(document.body, { | |
| childList: true, | |
| subtree: true | |
| }); | |
| </script> |
|
Cleaning up older PRs 💅 |
2 participants
ref AP-969