-
-
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 all 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,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(); | ||
|
@@ -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; | ||
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.