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 all commits
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
2 changes: 1 addition & 1 deletion app/globals.css
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,6 @@ body {

.onboarding-step .step-heading::before,
.onboarding-step h2::before {
content: "Step " counter(onboarding-step) ": ";
content: 'Step ' counter(onboarding-step) ': ';
font-weight: inherit;
}
7 changes: 6 additions & 1 deletion next.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {codecovNextJSWebpackPlugin} from '@codecov/nextjs-webpack-plugin';
import {withSentryConfig} from '@sentry/nextjs';

import {redirects} from './redirects.js';
import {REMOTE_IMAGE_PATTERNS} from './src/config/images';

const outputFileTracingExcludes = process.env.NEXT_PUBLIC_DEVELOPER_DOCS
? {
Expand Down Expand Up @@ -54,6 +55,10 @@ const nextConfig = {
trailingSlash: true,
serverExternalPackages: ['rehype-preset-minify'],
outputFileTracingExcludes,
images: {
contentDispositionType: 'inline', // "open image in new tab" instead of downloading
remotePatterns: REMOTE_IMAGE_PATTERNS,
},
webpack: (config, options) => {
config.plugins.push(
codecovNextJSWebpackPlugin({
Expand All @@ -71,7 +76,7 @@ const nextConfig = {
DEVELOPER_DOCS_: process.env.NEXT_PUBLIC_DEVELOPER_DOCS,
},
redirects,
rewrites: async () => [
rewrites: () => [
{
source: '/:path*.md',
destination: '/md-exports/:path*.md',
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
"@prettier/plugin-xml": "^3.3.1",
"@radix-ui/colors": "^3.0.0",
"@radix-ui/react-collapsible": "^1.1.1",
"@radix-ui/react-dialog": "^1.1.14",
"@radix-ui/react-dropdown-menu": "^2.1.2",
"@radix-ui/react-icons": "^1.3.2",
"@radix-ui/react-tabs": "^1.1.1",
Expand Down
131 changes: 95 additions & 36 deletions src/components/docImage.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,68 @@
import path from 'path';

import Image from 'next/image';

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

import {ImageLightbox} from './imageLightbox';

// Helper function to safely parse dimension values
const parseDimension = (value: string | number | undefined): number | undefined => {
if (typeof value === 'number' && value > 0 && value <= 10000) return value;
if (typeof value === 'string') {
const parsed = parseInt(value, 10);
return parsed > 0 && parsed <= 10000 ? parsed : undefined;
}
return undefined;
};

// Dimension pattern regex - used to identify dimension hashes like "800x600"
const DIMENSION_PATTERN = /^(\d+)x(\d+)$/;

// Helper function to extract hash from URL string (works with both relative and absolute URLs)
const extractHash = (url: string): string => {
const hashIndex = url.indexOf('#');
return hashIndex !== -1 ? url.slice(hashIndex + 1) : '';
};

// Helper function to check if a hash contains dimension information
const isDimensionHash = (hash: string): boolean => {
return DIMENSION_PATTERN.test(hash);
};

// Helper function to parse dimensions from URL hash
const parseDimensionsFromHash = (url: string): number[] => {
const hash = extractHash(url);
const match = hash.match(DIMENSION_PATTERN);

if (match) {
const width = parseInt(match[1], 10);
const height = parseInt(match[2], 10);
return width > 0 && width <= 10000 && height > 0 && height <= 10000
? [width, height]
: [];
}

return [];
};

// Helper function to remove dimension hash from URL while preserving fragment identifiers
const cleanUrl = (url: string): string => {
const hash = extractHash(url);

// If no hash or hash is not a dimension pattern, return original URL
if (!hash || !isDimensionHash(hash)) {
return url;
}

// Remove dimension hash
const hashIndex = url.indexOf('#');
return url.slice(0, hashIndex);
};

export default function DocImage({
src,
width: propsWidth,
height: propsHeight,
...props
}: Omit<React.HTMLProps<HTMLImageElement>, 'ref' | 'placeholder'>) {
const {path: pagePath} = serverContext();
Expand All @@ -14,44 +71,46 @@ export default function DocImage({
return null;
}

// Next.js Image component only supports images from the public folder
// or from a remote server with properly configured domain
if (src.startsWith('http')) {
// eslint-disable-next-line @next/next/no-img-element
return <img src={src} {...props} />;
}
const isExternal = isExternalImage(src);
let finalSrc = src;
let imgPath = src;

// If the image src is not an absolute URL, we assume it's a relative path
// and we prepend /mdx-images/ to it.
if (src.startsWith('./')) {
src = path.join('/mdx-images', src);
}
// account for the old way of doing things where the public folder structure mirrored the docs folder
else if (!src?.startsWith('/') && !src?.includes('://')) {
src = `/${pagePath.join('/')}/${src}`;
// For internal images, process the path
if (!isExternal) {
if (src.startsWith('./')) {
finalSrc = path.join('/mdx-images', src);
} else if (!src?.startsWith('/') && !src?.includes('://')) {
finalSrc = `/${pagePath.join('/')}/${src}`;
}

// For internal images, imgPath should be the pathname only
try {
const srcURL = new URL(finalSrc, 'https://example.com');
imgPath = srcURL.pathname;
} catch (_error) {
imgPath = finalSrc;
}
} else {
// For external images, clean URL by removing only dimension hashes, preserving fragment identifiers
finalSrc = cleanUrl(src);
imgPath = finalSrc;
Copy link

Choose a reason for hiding this comment

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

Bug: Image URL Hashes Incorrectly Stripped

For external image URLs, the code unconditionally strips the URL hash via cleanUrl before rendering and when opening in a new tab. This breaks valid fragment identifiers (e.g., image.svg#icon), causing images to render incorrectly or display unintended content, which is a change from previous behavior.

Fix in Cursor Fix in Web

}

// parse the size from the URL hash (set by remark-image-size.js)
const srcURL = new URL(src, 'https://example.com');
const imgPath = srcURL.pathname;
const [width, height] = srcURL.hash // #wxh
.slice(1)
.split('x')
.map(s => parseInt(s, 10));
// Parse dimensions from URL hash (works for both internal and external)
const hashDimensions = parseDimensionsFromHash(src);

// Use hash dimensions first, fallback to props
const width = hashDimensions[0] > 0 ? hashDimensions[0] : parseDimension(propsWidth);
const height = hashDimensions[1] > 0 ? hashDimensions[1] : parseDimension(propsHeight);

return (
<a href={imgPath} target="_blank" rel="noreferrer">
<Image
{...props}
src={src}
width={width}
height={height}
style={{
width: '100%',
height: 'auto',
}}
alt={props.alt ?? ''}
/>
</a>
<ImageLightbox
src={finalSrc}
imgPath={imgPath}
width={width}
height={height}
alt={props.alt ?? ''}
{...props}
/>
);
}
142 changes: 142 additions & 0 deletions src/components/imageLightbox/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
'use client';

import {useState} from 'react';
import Image from 'next/image';

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

interface ImageLightboxProps
extends Omit<
React.HTMLProps<HTMLImageElement>,
'ref' | 'src' | 'width' | 'height' | 'alt'
> {
alt: string;
imgPath: string;
src: string;
height?: number;
width?: number;
}

const getImageUrl = (src: string, imgPath: string): string => {
if (isExternalImage(src)) {
// Normalize protocol-relative URLs to use https:
return src.startsWith('//') ? `https:${src}` : src;
}
return imgPath;
};

type ValidDimensions = {
height: number;
width: number;
};

const getValidDimensions = (width?: number, height?: number): ValidDimensions | null => {
if (
width != null &&
height != null &&
!isNaN(width) &&
!isNaN(height) &&
width > 0 &&
height > 0
) {
return {width, height};
}
return null;
};

export function ImageLightbox({
src,
alt,
width,
height,
imgPath,
style,
className,
...props
}: ImageLightboxProps) {
const [open, setOpen] = useState(false);

const dimensions = getValidDimensions(width, height);
const shouldUseNextImage =
!!dimensions && (!isExternalImage(src) || isAllowedRemoteImage(src));

const openInNewTab = () => {
window.open(getImageUrl(src, imgPath), '_blank', 'noopener,noreferrer');
};

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();
openInNewTab();
}
// Regular click is handled by Dialog.Trigger
};

const handleKeyDown = (e: React.KeyboardEvent) => {
// Ctrl/Cmd+Enter/Space opens in new tab
// Regular Enter/Space is handled by Dialog.Trigger
if ((e.key === 'Enter' || e.key === ' ') && (e.ctrlKey || e.metaKey)) {
e.preventDefault();
e.stopPropagation();
openInNewTab();
}
};

// 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;

const renderImage = (isInline: boolean = true) => {
const renderedSrc = getImageUrl(src, imgPath);
const imageClassName = isInline
? className
: 'max-h-[90vh] max-w-[90vw] object-contain';
const imageStyle = isInline
? {width: '100%', height: 'auto', ...style}
: {width: 'auto', height: 'auto'};

if (shouldUseNextImage && dimensions) {
return (
<Image
src={renderedSrc}
width={dimensions.width}
height={dimensions.height}
style={imageStyle}
className={imageClassName}
alt={alt}
priority={!isInline}
{...imageCompatibleProps}
/>
);
}
return (
/* eslint-disable-next-line @next/next/no-img-element */
<img
src={renderedSrc}
alt={alt}
loading={isInline ? 'lazy' : 'eager'}
decoding="async"
style={imageStyle}
className={imageClassName}
{...imageCompatibleProps}
/>
);
};

return (
<Lightbox.Root open={open} onOpenChange={setOpen} content={renderImage(false)}>
<Lightbox.Trigger
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}`}
>
{renderImage()}
</Lightbox.Trigger>
</Lightbox.Root>
Comment on lines +130 to +140
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

);
}
Loading
Loading