Skip to content

Update image handling #14564

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
76137dd
Add image lightbox with Radix UI Dialog and improved image handling
cursoragent Aug 5, 2025
222fbb2
Refactor DocImage components and improve type ordering
cursoragent Aug 5, 2025
1d4c770
[getsentry/action-github-commit] Auto commit
getsantry[bot] Aug 5, 2025
970049b
remove alt text overlay
jaffrepaul Aug 6, 2025
2e98bdc
fix default browser behavior
jaffrepaul Aug 6, 2025
041b40d
more specific classes
jaffrepaul Aug 6, 2025
b5470a6
fix external image bug, update to pass all props, & ensure fallback s…
jaffrepaul Aug 6, 2025
12ca95c
refactor to fix event handling & propagation issues
jaffrepaul Aug 6, 2025
a636625
[getsentry/action-github-commit] Auto commit
getsantry[bot] Aug 6, 2025
11227e4
remove anchors
jaffrepaul Aug 6, 2025
528b31a
prevent browser context menu from downloading images instead of openi…
jaffrepaul Aug 6, 2025
0bd0bae
fix window parameter & click behavior cursor bug
jaffrepaul Aug 7, 2025
37fd8ce
[getsentry/action-github-commit] Auto commit
getsantry[bot] Aug 7, 2025
19db25b
cleanup
jaffrepaul Aug 7, 2025
7b781f4
add basic a11y
jaffrepaul Aug 7, 2025
106d776
[getsentry/action-github-commit] Auto commit
getsantry[bot] Aug 7, 2025
50455a3
update button styles
jaffrepaul Aug 7, 2025
6c6b8a1
refactor to simplify logic
jaffrepaul Aug 7, 2025
19e598e
prop and types improvements
jaffrepaul Aug 7, 2025
c74e563
[getsentry/action-github-commit] Auto commit
getsantry[bot] Aug 7, 2025
9bffc3c
remove DocImageClient to simplify and scope styles
jaffrepaul Aug 8, 2025
4065b0c
port image logic to imageLightbox
jaffrepaul Aug 8, 2025
c68596c
dry things up
jaffrepaul Aug 8, 2025
30c260b
fix alt prop bug
jaffrepaul Aug 8, 2025
5a7c1be
[getsentry/action-github-commit] Auto commit
getsantry[bot] Aug 8, 2025
7ff34e8
fix type narrowing issue
jaffrepaul Aug 8, 2025
19b1e92
extend next config for external images
jaffrepaul Aug 8, 2025
02a849e
[getsentry/action-github-commit] Auto commit
getsantry[bot] Aug 8, 2025
ca1f744
gpt5 update 💪
jaffrepaul Aug 8, 2025
d6081b0
[getsentry/action-github-commit] Auto commit
getsantry[bot] Aug 8, 2025
896228e
fix image edgecase bug
jaffrepaul Aug 8, 2025
052325c
add PR feedback
jaffrepaul Aug 8, 2025
0770228
better abstraction refactor & cleanup
jaffrepaul Aug 8, 2025
4be0b7e
Update package.json
jaffrepaul Aug 8, 2025
689e57f
bugbot smash
jaffrepaul Aug 8, 2025
5dc9bcd
delegate basic events to radix, keep modifier logic
jaffrepaul Aug 8, 2025
b2fecbf
bugbot fix
jaffrepaul Aug 9, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/components/docImage.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import path from 'path';

import {isExternalImage} from 'sentry-docs/config/images';
import {serverContext} from 'sentry-docs/serverContext';

import {ImageLightbox} from './imageLightbox';
Expand Down Expand Up @@ -64,12 +65,12 @@ export default function DocImage({
return null;
}

const isExternalImage = src.startsWith('http') || src.startsWith('//');
const isExternal = isExternalImage(src);
let finalSrc = src;
let imgPath = src;

// For internal images, process the path
if (!isExternalImage) {
if (!isExternal) {
if (src.startsWith('./')) {
finalSrc = path.join('/mdx-images', src);
} else if (!src?.startsWith('/') && !src?.includes('://')) {
Expand Down
60 changes: 23 additions & 37 deletions src/components/imageLightbox/index.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
'use client';

import {useState} from 'react';
import * as Dialog from '@radix-ui/react-dialog';
import Image from 'next/image';

import {Lightbox} from 'sentry-docs/components/lightbox';
import {isAllowedRemoteImage} from 'sentry-docs/config/images';
import {isAllowedRemoteImage, isExternalImage} from 'sentry-docs/config/images';

interface ImageLightboxProps
extends Omit<
Expand All @@ -19,15 +18,9 @@ interface ImageLightboxProps
width?: number;
}

// Helper functions
const isExternalImage = (src: string): boolean =>
src.startsWith('http') || src.startsWith('//');

const getImageUrl = (src: string, imgPath: string): string =>
isExternalImage(src) ? src : imgPath;

// Using shared allowlist logic from src/config/images

type ValidDimensions = {
height: number;
width: number;
Expand Down Expand Up @@ -59,46 +52,36 @@ export function ImageLightbox({
}: ImageLightboxProps) {
const [open, setOpen] = useState(false);

// Check if we should use Next.js Image or regular img
// Use Next.js Image for internal images with valid dimensions
// Use regular img for external images or when dimensions are invalid/missing
const dimensions = getValidDimensions(width, height);
const shouldUseNextImage =
!!dimensions && (!isExternalImage(src) || isAllowedRemoteImage(src));

const handleModifierClick = (e: React.MouseEvent) => {
// If Ctrl/Cmd+click, open image in new tab instead of lightbox
if (e.ctrlKey || e.metaKey) {
const openInNewTab = () => {
window.open(getImageUrl(src, imgPath), '_blank');
};

const handleClick = (e: React.MouseEvent) => {
// Middle-click or Ctrl/Cmd+click opens in new tab
if (e.button === 1 || e.ctrlKey || e.metaKey) {
e.preventDefault();
e.stopPropagation();
const url = getImageUrl(src, imgPath);
const newWindow = window.open(url, '_blank');
if (newWindow) {
newWindow.opener = null; // Security: prevent opener access
}
openInNewTab();
return;
}
// Normal click will be handled by Dialog.Trigger
// Regular click falls through to Dialog.Trigger
};

const handleModifierKeyDown = (e: React.KeyboardEvent) => {
// Handle Ctrl/Cmd+Enter or Ctrl/Cmd+Space to open in new tab
const handleKeyDown = (e: React.KeyboardEvent) => {
if ((e.key === 'Enter' || e.key === ' ') && (e.ctrlKey || e.metaKey)) {
e.preventDefault();
e.stopPropagation();
const url = getImageUrl(src, imgPath);
const newWindow = window.open(url, '_blank');
if (newWindow) {
newWindow.opener = null; // Security: prevent opener access
}
openInNewTab();
}
// Normal key presses will be handled by Dialog.Trigger
// Regular Enter/Space falls through to Dialog.Trigger
};

// Filter out props that are incompatible with Next.js Image component
// Next.js Image has stricter typing for certain props like 'placeholder'
const {placeholder: _placeholder, ...imageCompatibleProps} = props;

// Render the appropriate image component
const renderImage = (isInline: boolean = true) => {
const renderedSrc = getImageUrl(src, imgPath);
const imageClassName = isInline
Expand Down Expand Up @@ -127,27 +110,30 @@ export function ImageLightbox({
<img
src={renderedSrc}
alt={alt}
loading={isInline ? 'lazy' : 'lazy'}
loading={isInline ? 'lazy' : 'eager'}
decoding="async"
style={imageStyle}
className={imageClassName}
{...props}
{...imageCompatibleProps}
/>
);
};

return (
<Lightbox.Root open={open} onOpenChange={setOpen} content={renderImage(false)}>
<Dialog.Trigger asChild>
<Lightbox.Trigger asChild>
<div
onClick={handleModifierClick}
onKeyDown={handleModifierKeyDown}
onClick={handleClick}
onAuxClick={handleClick}
onKeyDown={handleKeyDown}
className="cursor-pointer border-none bg-transparent p-0 block w-full no-underline"
aria-label={`View image: ${alt}`}
role="button"
tabIndex={0}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont think it needs this (apart from aira-label) because https://github.com/radix-ui/primitives/blob/main/packages/react/dialog/src/dialog.tsx#L105-L107 handles it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nice. Less code is the best code! I still need some of it for the modifier logic but can pull some off

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup and you can put all that on the <Dialog.Trigger /> directly and remove the asChild prop

>
{renderImage()}
</div>
</Dialog.Trigger>
</Lightbox.Trigger>
</Lightbox.Root>
);
}
70 changes: 17 additions & 53 deletions src/components/lightbox/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,49 +8,38 @@
* </Lightbox.Root>
*
* @example
* // Advanced usage with custom trigger and controlled state
* // Advanced usage with Lightbox.Trigger and controlled state
* const [open, setOpen] = useState(false);
*
* <Lightbox.Root
* open={open}
* onOpenChange={setOpen}
* content={<MyLargeContent />}
* trigger={
* <Lightbox.Trigger onClick={handleClick}>
* <Lightbox.Root open={open} onOpenChange={setOpen} content={<MyLargeContent />}>
* <Lightbox.Trigger asChild>
* <button onClick={handleClick}>
* <MyThumbnail />
* </Lightbox.Trigger>
* }
* />
* </button>
* </Lightbox.Trigger>
* </Lightbox.Root>
*/

'use client';

import {useState} from 'react';
import {Fragment, useState} from 'react';
import {X} from 'react-feather';
import * as Dialog from '@radix-ui/react-dialog';

import styles from './lightbox.module.scss';

interface LightboxProps {
children?: React.ReactNode;
content: React.ReactNode;
trigger?: React.ReactNode;
children?: React.ReactNode;
closeButton?: boolean;
onOpenChange?: (open: boolean) => void;
open?: boolean;
closeButton?: boolean;
trigger?: React.ReactNode;
}

interface LightboxTriggerProps {
children: React.ReactNode;
onClick?: (e: React.MouseEvent) => void;
onKeyDown?: (e: React.KeyboardEvent) => void;
className?: string;
'aria-label'?: string;
}

interface LightboxContentProps {
children: React.ReactNode;
className?: string;
asChild?: boolean;
}

interface LightboxCloseProps {
Expand Down Expand Up @@ -92,54 +81,29 @@ function LightboxRoot({
);
}

// Trigger component for custom triggers
function LightboxTrigger({
children,
onClick,
onKeyDown,
className = '',
'aria-label': ariaLabel,
}: LightboxTriggerProps) {
return (
<div
role="button"
tabIndex={0}
className={`cursor-pointer border-none bg-transparent p-0 block w-full no-underline ${className}`}
onClick={onClick}
onKeyDown={onKeyDown}
aria-label={ariaLabel}
>
{children}
</div>
);
}

// Content wrapper (optional, for additional styling)
function LightboxContent({children, className = ''}: LightboxContentProps) {
return <div className={className}>{children}</div>;
// Trigger component
function LightboxTrigger({children, asChild = false}: LightboxTriggerProps) {
return <Dialog.Trigger asChild={asChild}>{children}</Dialog.Trigger>;
}

// Close button component
function LightboxClose({children, className = ''}: LightboxCloseProps) {
return (
<Dialog.Close className={className}>
{children || (
<>
<Fragment>
<X className="h-4 w-4 stroke-[2.5]" />
<span className="sr-only">Close</span>
</>
</Fragment>
)}
</Dialog.Close>
);
}

// Compound component exports
export const Lightbox = {
Root: LightboxRoot,
Trigger: LightboxTrigger,
Content: LightboxContent,
Close: LightboxClose,
};

// For backward compatibility, export the root as default
export default LightboxRoot;
8 changes: 7 additions & 1 deletion src/config/images.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,15 @@ export const REMOTE_IMAGE_PATTERNS = REMOTE_IMAGE_HOSTNAMES.map(hostname => ({
hostname,
}));

export function isExternalImage(src: string): boolean {
return src.startsWith('http') || src.startsWith('//');
}

export function isAllowedRemoteImage(src: string): boolean {
try {
const url = new URL(src);
// Handle protocol-relative URLs by adding https: protocol
const normalizedSrc = src.startsWith('//') ? `https:${src}` : src;
const url = new URL(normalizedSrc);
return (
url.protocol === 'https:' &&
(REMOTE_IMAGE_HOSTNAMES as readonly string[]).includes(url.hostname)
Expand Down
2 changes: 1 addition & 1 deletion src/remark-image-size.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export default function remarkImageSize(options) {
return tree =>
visit(tree, 'image', node => {
// don't process external images
if (node.url.startsWith('http')) {
if (node.url.startsWith('http') || node.url.startsWith('//')) {
return;
}
const fullImagePath = path.join(
Expand Down