-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Update image handling #14564
Changes from 36 commits
76137dd
222fbb2
1d4c770
970049b
2e98bdc
041b40d
b5470a6
12ca95c
a636625
11227e4
528b31a
0bd0bae
37fd8ce
19db25b
7b781f4
106d776
50455a3
6c6b8a1
19e598e
c74e563
9bffc3c
4065b0c
c68596c
30c260b
5a7c1be
7ff34e8
19b1e92
02a849e
ca1f744
d6081b0
896228e
052325c
0770228
4be0b7e
689e57f
5dc9bcd
b2fecbf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,62 @@ | ||
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; | ||
}; | ||
|
||
// Helper function to parse dimensions from URL hash | ||
const parseDimensionsFromHash = (url: string): number[] => { | ||
try { | ||
const urlObj = new URL(url, 'https://example.com'); | ||
const hash = urlObj.hash.slice(1); | ||
|
||
// Only parse hash if it looks like dimensions (e.g., "800x600", "100x200") | ||
// Must be exactly two positive integers separated by 'x' | ||
const dimensionPattern = /^(\d+)x(\d+)$/; | ||
const match = hash.match(dimensionPattern); | ||
|
||
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 []; | ||
} catch (_error) { | ||
return []; | ||
} | ||
}; | ||
|
||
// Helper function to clean URL by removing hash | ||
const cleanUrl = (url: string): string => { | ||
try { | ||
const urlObj = new URL(url); | ||
// For external URLs, reconstruct without hash | ||
urlObj.hash = ''; | ||
return urlObj.toString(); | ||
} catch (_error) { | ||
// If URL parsing fails, just remove hash manually | ||
return url.split('#')[0]; | ||
} | ||
}; | ||
|
||
export default function DocImage({ | ||
src, | ||
width: propsWidth, | ||
height: propsHeight, | ||
...props | ||
}: Omit<React.HTMLProps<HTMLImageElement>, 'ref' | 'placeholder'>) { | ||
const {path: pagePath} = serverContext(); | ||
|
@@ -14,44 +65,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, strip hash from both src and imgPath | ||
finalSrc = cleanUrl(src); | ||
imgPath = finalSrc; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Image URL Hashes Incorrectly StrippedFor external image URLs, the code unconditionally strips the URL hash via |
||
} | ||
|
||
// 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} | ||
/> | ||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
); | ||
} |
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 | ||
}; | ||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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} | ||
/> | ||
); | ||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice |
||
); | ||
} |
Uh oh!
There was an error while loading. Please reload this page.