[UXIT-1731] Validate Image Src #919

barbaraperic · 2024-12-16T20:41:10Z

📝 Description

Type: Refactor

This PR introduces SmartImage wrapper component (name inspired by SmartTextLink). The purpose of the component is to handle image URL validity and conditionally render the fallback.

🛠️ Key Changes

Currently it is used in Card, PageHeader (app/components) , ArticleHeader, PostHeader (ecosystem_explorer/components). These components require fallback handling because images in markdown frontmatter are optional.

With use of this component we can remove the "image || graphicsData.imageFallback.data" condition and just pass the image as a prop.

SmartImage will render the fallback (currently fixed but can be customizable in the future) in the following cases:

The image is missing in the markdown frontmatter.
The src URL in the markdown frontmatter is invalid.
A remote image URL is passed, but the URL is broken.

🧪 How to Test

Check if the fallback is shown in the cases described above.

notion-workspace · 2024-12-16T20:41:13Z

[FF] Validate Image Src

vercel · 2024-12-16T20:41:15Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
filecoin-foundation-site	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 20, 2024 9:06am

barbaraperic · 2024-12-16T22:26:58Z

src/app/_components/Card.tsx

        {...commonProps}
        className={clsx(commonProps.className, 'aspect-video')}
-        src={image.data}
+        src={(image.data as StaticImageData).src}


Typescript is complaining here although we have a isStaticImage condition (which check data in the image)

It should just be image.src no? image.data doesn't exist on SmartImageProps

Sorry not sure I understand 😕 StaticImages have data key, like this:

{ data: { src: '/_next/static/media/022624-ff-anualreport.b5deae8f.png', height: 1536, width: 3072, blurDataURL: '/_next/image?url=%2F_next%2Fstatic%2Fmedia%2F022624-ff-anualreport.b5deae8f.png&w=8&q=70', blurWidth: 8, blurHeight: 4 }, alt: '', sizes: '(min-width: 640px) 710px, (min-width: 768px) 980px, (min-width: 1024px) 480px, 100vw' }

Ah so this should actually be image.data...

barbaraperic · 2024-12-16T22:29:49Z

src/app/_components/SmartImage.tsx

@@ -0,0 +1,86 @@
+'use server'


we need to be explicit to run on the server because fs and path exclusively run in the node env

Aren't components 'use server' by default?

I haven't seen this used in components. What does it do?

Basically makes sure it runs server side. For example, in GovernanceCalendarCard (use client component) we import Card component which uses SmartImage. if we don't explicitly write use server in the SmartImage component, it will run on the client, and in that case we get an error because we use fs which can't run in the browser.

CharlyMartin

I did a first pass with quite a few questions / suggestions.

Happy to have another look once the comments are resolved.

Thanks Barbara!

PS: Should statically imported images be part of the smart image component? Since we already know these are valid, it might not be necessary.

CharlyMartin · 2024-12-17T16:37:39Z

src/app/_components/SmartImage.tsx

@@ -0,0 +1,86 @@
+'use server'


Aren't components 'use server' by default?

I haven't seen this used in components. What does it do?

CharlyMartin · 2024-12-17T16:51:00Z

src/app/_components/SmartImage.tsx

+async function checkAssetImageExists(src: string) {
+  const publicPath = path.join(process.cwd(), 'public', src)
+  try {
+    await fs.access(publicPath, fs.constants.F_OK)
+    return true
+  } catch {
+    return false
+  }
+}


This reads a bit better maybe?

Suggested change

async function checkAssetImageExists(src: string) {

const publicPath = path.join(process.cwd(), 'public', src)

try {

await fs.access(publicPath, fs.constants.F_OK)

return true

} catch {

return false

}

}

async function checkAssetImageExists(src: string) {

const publicPath = path.join(process.cwd(), 'public', src)

const stats = await fs.stat(publicPath)

return stats.isFile()

}

Ah yes, I like how async await reads, but this made me think we could use try catch to capture errors? what do you think?

async function checkAssetImageExists(src: string) { const publicPath = path.join(process.cwd(), 'public', src); try { await fs.access(publicPath, fs.constants.F_OK); return true; } catch (error) { console.error(`Error checking asset image existence at path "${publicPath}":`, error); return false; } }

CharlyMartin · 2024-12-17T16:52:24Z

src/app/_components/SmartImage.tsx

+export type SmartImageProps = {
+  src?: string | StaticImageProps['data']
+  alt?: string
+  className?: string


className?: string can be included in Omit<ImageProps, 'src' | 'alt'>

Hm it's included in the line 21? Or you're referring to something else?

CharlyMartin · 2024-12-17T16:52:41Z

src/app/_components/SmartImage.tsx

+import { type ImageProps } from 'next/image'
+import Image from 'next/image'


Group imports?

CharlyMartin · 2024-12-17T16:55:43Z

src/app/_components/SmartImage.tsx

+
+export async function SmartImage({
+  src,
+  alt = '',


Why do we make alt an empty string if undefined? I don't understand 🙃

CharlyMartin · 2024-12-17T17:03:56Z

src/app/_components/SmartImage.tsx

+  alt?: string
+  className?: string
+  objectFit?: ImageObjectFit
+  padding?: boolean


What does padding do?

we use it in the Card component, same as objectFit

CharlyMartin · 2024-12-17T17:09:54Z

src/app/_components/SmartImage.tsx

+  padding?: boolean
+} & Omit<ImageProps, 'src' | 'alt' | 'className'>
+
+export async function SmartImage({


What do you think about something like this?

Does it read a bit better to you?

export async function SmartImage({ src, alt = '', ...props }: SmartImageProps) { if (!src) { return <Image src={fallbackSrc} alt={fallbackAlt} {...props} /> } const isValid = await checkImageValidity(src) return ( <Image src={isValid ? src : fallbackSrc} alt={isValid ? alt : fallbackAlt} {...props} /> ) } async function checkImageValidity(src: NonNullable<SmartImageProps['src']>) { const isStaticImport = typeof src === 'object' const isAssetImage = typeof src === 'string' && src.startsWith('/assets') if (isStaticImport) { return true } if (isAssetImage) { return await checkAssetImageExists(src) } return await checkRemoteImageExists(src) }

I see what you mean, personally I kind of like the separation of logic (getImageSrc) and ui rendering (SmartImage). That said, I’m happy to explore versions if you think this doesn’t work well for the current context.

CharlyMartin · 2024-12-17T17:22:50Z

src/app/_components/Card.tsx

        {...commonProps}
        className={clsx(commonProps.className, 'aspect-video')}
-        src={image.data}
+        src={(image.data as StaticImageData).src}


It should just be image.src no? image.data doesn't exist on SmartImageProps

barbaraperic · 2024-12-19T13:26:45Z

PS: Should statically imported images be part of the smart image component? Since we already know these are valid, it might not be necessary.

Yes, good observation!

- Simplify SmartImage component and improve type safety - Remove redundant static image handling logic

Create image component that handles fallback is src is invalid

5575a46

barbaraperic self-assigned this Dec 16, 2024

vercel bot had a problem deploying to Preview December 16, 2024 20:42 Failure

Fix imports

4d1f3e7

vercel bot had a problem deploying to Preview December 16, 2024 20:47 Failure

Use use server for Smart Image component

43d75d8

vercel bot had a problem deploying to Preview December 16, 2024 21:58 Failure

Fix required alt prop

9b79ada

vercel bot deployed to Preview December 16, 2024 22:04 View deployment

barbaraperic requested a review from mirhamasala December 16, 2024 22:24

barbaraperic commented Dec 16, 2024

View reviewed changes

Remove the return type

3176bb6

vercel bot deployed to Preview December 16, 2024 22:36 View deployment

barbaraperic marked this pull request as ready for review December 17, 2024 13:38

mirhamasala requested review from CharlyMartin and removed request for mirhamasala December 17, 2024 14:11

CharlyMartin requested changes Dec 17, 2024

View reviewed changes

Merge branch 'main' into bp/validate-image-src

667e4c3

vercel bot deployed to Preview December 19, 2024 12:19 View deployment

fix: correct image source handling in Card component

15b548b

vercel bot deployed to Preview December 19, 2024 12:32 View deployment

- Replace SmartImage with Next.js Image component in Card and PageHeader

60c78d2

- Simplify SmartImage component and improve type safety - Remove redundant static image handling logic

barbaraperic requested a review from CharlyMartin December 19, 2024 13:29

vercel bot had a problem deploying to Preview December 19, 2024 13:31 Failure

Rollback to where alt is possibly undefined in SmartImage

c5543cd

vercel bot had a problem deploying to Preview December 19, 2024 13:47 Failure

fix: add fallback for empty alt text in Card Image component

3012340

vercel bot had a problem deploying to Preview December 19, 2024 14:50 Failure

fix: add fallback for empty alt text in PageHeader Image component

ba2300c

vercel bot deployed to Preview December 19, 2024 15:00 View deployment

Merge branch 'main' into bp/validate-image-src

2c75747

vercel bot deployed to Preview December 20, 2024 09:06 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[UXIT-1731] Validate Image Src #919

[UXIT-1731] Validate Image Src #919

barbaraperic commented Dec 16, 2024 •

edited

Loading

notion-workspace bot commented Dec 16, 2024

vercel bot commented Dec 16, 2024 •

edited

Loading

barbaraperic Dec 16, 2024

CharlyMartin Dec 17, 2024

barbaraperic Dec 19, 2024

barbaraperic Dec 16, 2024 •

edited

Loading

CharlyMartin Dec 17, 2024

barbaraperic Dec 19, 2024 •

edited

Loading

CharlyMartin left a comment •

edited

Loading

CharlyMartin Dec 17, 2024

CharlyMartin Dec 17, 2024

barbaraperic Dec 19, 2024 •

edited

Loading

CharlyMartin Dec 17, 2024

barbaraperic Dec 19, 2024

CharlyMartin Dec 17, 2024

CharlyMartin Dec 17, 2024

CharlyMartin Dec 17, 2024

barbaraperic Dec 19, 2024 •

edited

Loading

CharlyMartin Dec 17, 2024

barbaraperic Dec 19, 2024

CharlyMartin Dec 17, 2024

barbaraperic commented Dec 19, 2024

		import { type ImageProps } from 'next/image'
		import Image from 'next/image'

[UXIT-1731] Validate Image Src #919

Are you sure you want to change the base?

[UXIT-1731] Validate Image Src #919

Conversation

barbaraperic commented Dec 16, 2024 • edited Loading

📝 Description

🛠️ Key Changes

🧪 How to Test

notion-workspace bot commented Dec 16, 2024

vercel bot commented Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barbaraperic Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barbaraperic Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

CharlyMartin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barbaraperic Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barbaraperic Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barbaraperic commented Dec 19, 2024

barbaraperic commented Dec 16, 2024 •

edited

Loading

vercel bot commented Dec 16, 2024 •

edited

Loading

barbaraperic Dec 16, 2024 •

edited

Loading

barbaraperic Dec 19, 2024 •

edited

Loading

CharlyMartin left a comment •

edited

Loading

barbaraperic Dec 19, 2024 •

edited

Loading

barbaraperic Dec 19, 2024 •

edited

Loading