perf(media): update image handling and dependencies - #682
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vivek-gite The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
✅ Deploy Preview for project-hami ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Welcome @vivek-gite! It looks like this is your first PR to Project-HAMi/website 🎉 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a build-time responsive image pipeline with Sharp, size validation, responsive rendering, updated logo and blog content integration, and compatibility changes for figures and lightbox behavior. ChangesResponsive image delivery
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SiteContent
participant ResponsiveImage
participant Browser
participant Lightbox
SiteContent->>ResponsiveImage: pass image source and sizes
ResponsiveImage->>Browser: render picture with responsive sources
Browser->>Lightbox: open clicked image
Lightbox->>Browser: load original lightbox source
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
package.json (1)
16-25: 🚀 Performance & Scalability | 🔵 TrivialFull regenerate-on-every-run cost will grow with the image set.
images:generatewipes and rebuilds the entirestatic/img/responsivedirectory on everyprestart/prebuild(per scripts/generate-responsive-images.js line 74), regardless of whether source assets changed. Fine for the current 2-image scope; worth revisiting with content-hash-based incremental generation once more images are migrated to the responsive pipeline, to keep local dev/build startup fast.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` around lines 16 - 25, Update the image preparation flow around images:generate and generate-responsive-images.js so repeated prestart/prebuild runs skip regeneration when source assets are unchanged. Add content-hash-based change detection while preserving full generation when inputs change, and ensure the existing images:check and responsive output behavior remains intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@blog/hami-at-kubecon-eu-2026/index.md`:
- Line 33: Rename the English blog post directory to the required
YYYY-MM-DD-title format, and rename or update its localized counterpart to use
the matching dated title structure. Preserve the post content and ensure all
references to the old directory remain aligned with the new paths.
---
Nitpick comments:
In `@package.json`:
- Around line 16-25: Update the image preparation flow around images:generate
and generate-responsive-images.js so repeated prestart/prebuild runs skip
regeneration when source assets are unchanged. Add content-hash-based change
detection while preserving full generation when inputs change, and ensure the
existing images:check and responsive output behavior remains intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f1a7c5ff-d367-46b3-bb6c-cfb524591adb
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
.gitignoreblog/hami-at-kubecon-eu-2026/index.mdi18n/zh/docusaurus-plugin-content-blog/hami-at-kubecon-eu-2026/index.mdpackage.jsonscripts/check-image-sizes.jsscripts/generate-responsive-images.jsscripts/image-size-allowlist.jsonsrc/client/imageFigureNumber.jssrc/components/ResponsiveImage.jssrc/components/logoWall.jssrc/css/custom.csssrc/data/responsive-images.jsonsrc/theme/utils/useImageLightbox.js
- Added new scripts for image size checking and generation in package.json. - Introduced the 'sharp' library for image processing. - Updated .gitignore to exclude responsive image directory. - Modified blog and i18n markdown files to use ResponsiveImage component for better image handling. - Enhanced logoWall component to utilize ResponsiveImage for logos. - Adjusted CSS for support wrapper to ensure proper image display. - Fixed image lightbox functionality to support data attributes for image sources. These changes improve image management and enhance the overall performance of the application. Signed-off-by: Gite Vivek Kumar <vivekkumargite@outlook.com>
b59d2f3 to
7eb9a23
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check-image-sizes.js`:
- Line 11: Update the supportedExtensions set in check-image-sizes.js to include
the .avif and .gif extensions, ensuring those raster formats are included in the
existing image size validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e4862d5-69f0-4c12-8fe7-bd85c41e6c39
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
.gitignoreblog/hami-at-kubecon-eu-2026/index.mdi18n/zh/docusaurus-plugin-content-blog/hami-at-kubecon-eu-2026/index.mdpackage.jsonscripts/check-image-sizes.jsscripts/generate-responsive-images.jsscripts/image-size-allowlist.jsonsrc/client/imageFigureNumber.jssrc/components/ResponsiveImage.jssrc/components/logoWall.jssrc/css/custom.csssrc/data/responsive-images.jsonsrc/theme/utils/useImageLightbox.js
🚧 Files skipped from review as they are similar to previous changes (11)
- src/client/imageFigureNumber.js
- scripts/image-size-allowlist.json
- blog/hami-at-kubecon-eu-2026/index.md
- src/components/logoWall.js
- package.json
- src/components/ResponsiveImage.js
- src/data/responsive-images.json
- src/css/custom.css
- i18n/zh/docusaurus-plugin-content-blog/hami-at-kubecon-eu-2026/index.md
- src/theme/utils/useImageLightbox.js
- scripts/generate-responsive-images.js
| const rootDirectory = path.resolve(__dirname, ".."); | ||
| const staticDirectory = path.join(rootDirectory, "static"); | ||
| const responsiveDirectory = path.join(staticDirectory, "img", "responsive"); | ||
| const supportedExtensions = new Set([".jpeg", ".jpg", ".png", ".webp"]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include AVIF and GIF in the raster size check.
These raster formats currently bypass validation, so oversized assets can be added under static/ without an exemption.
Proposed fix
-const supportedExtensions = new Set([".jpeg", ".jpg", ".png", ".webp"]);
+const supportedExtensions = new Set([
+ ".avif",
+ ".gif",
+ ".jpeg",
+ ".jpg",
+ ".png",
+ ".webp",
+]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const supportedExtensions = new Set([".jpeg", ".jpg", ".png", ".webp"]); | |
| const supportedExtensions = new Set([ | |
| ".avif", | |
| ".gif", | |
| ".jpeg", | |
| ".jpg", | |
| ".png", | |
| ".webp", | |
| ]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/check-image-sizes.js` at line 11, Update the supportedExtensions set
in check-image-sizes.js to include the .avif and .gif extensions, ensuring those
raster formats are included in the existing image size validation.
mesutoezdil
left a comment
mesutoezdil
left a comment
There was a problem hiding this comment.
checked the deploy preview, the variants resolve and the srcset looks right, few notes below.
title says chore: but this is a perf change, pls retitle to match the issue (perf(media): ...).
the pr description doesn't follow the repo template, pls fill the 'what this PR does / why we need it' section.
no ai assistance disclosure is present. if any ai tool was used, it must be disclosed per CONTRIBUTING.md: https://github.com/Project-HAMi/HAMi/blob/master/CONTRIBUTING.md#ai-assistance-notice
| } | ||
|
|
||
| async function main() { | ||
| const images = await findRasterImages(staticDirectory); |
There was a problem hiding this comment.
this walks all 194 rasters in static/ on every prestart, pls keep it in ci only so npm start doesn't wait on it.
There was a problem hiding this comment.
Updated images:prepare so local start/build hooks only generate responsive variants. The full raster-size scan now runs as an explicit Docs Health CI step; it remains available locally via npm run images:check.
| }), | ||
| ); | ||
|
|
||
| if (violations.length > 0 || staleExemptions.size > 0) { |
There was a problem hiding this comment.
a stale exemption shouldn't fail the build, deleting or shrinking an allowlisted img would break everyone's npm start.
| const sourcePath = path.join(staticDirectory, src.replace(/^\//, "")); | ||
| const metadata = await sharp(sourcePath).metadata(); | ||
|
|
||
| if (metadata.width !== config.width || metadata.height !== config.height) { |
There was a problem hiding this comment.
this breaks the build if anyone re-exports the src img, pls read the dims from the file instead of asserting them.
| import useBaseUrl from "@docusaurus/useBaseUrl"; | ||
| import responsiveImages from "../data/responsive-images.json"; | ||
|
|
||
| function getVariantPath(src, width, extension) { |
There was a problem hiding this comment.
same getVariantPath is copied in scripts/generate-responsive-images.js, if either drifts the srcset 404s silently, pls share one helper.
| "static/img/adopters/ecloud.jpg": "Existing adopter logo; migrate through the responsive pipeline before changing it.", | ||
| "static/img/adopters/linkedin.png": "Existing adopter logo; migrate through the responsive pipeline before changing it.", | ||
| "static/img/adopters/pingan-securities.png": "Existing adopter logo; migrate through the responsive pipeline before changing it.", | ||
| "static/img/adopters/szzt.png": "Original retained for quality; card-sized variants are generated at build time.", |
There was a problem hiding this comment.
nothing serves the 15000px szzt original since the biggest variant is 340px, pls downscale the src and drop this exemption.
| "static/img/adopters/ecloud.jpg": "Existing adopter logo; migrate through the responsive pipeline before changing it.", | ||
| "static/img/adopters/linkedin.png": "Existing adopter logo; migrate through the responsive pipeline before changing it.", | ||
| "static/img/adopters/pingan-securities.png": "Existing adopter logo; migrate through the responsive pipeline before changing it.", |
There was a problem hiding this comment.
these logos got an exemption instead of the new pipeline, so #662 is only fixed for szzt.
vivek-gite
changed the title
vivek-gite
changed the title
…nsive variants. The full raster-size scan now runs as an explicit Docs Health CI step; it remains available locally via npm run images:check.
|
Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits. 📝 Please follow instructions in the contributing guide to update your commits with the DCO Full details of the Developer Certificate of Origin can be found at developercertificate.org. The list of commits missing DCO signoff:
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
What type of PR is this?
feature
What this PR does / why we need it:
Implements the responsive-image work from #662 so visitors download image candidates appropriate to the rendered size instead of always receiving large source files.
ResponsiveImagecomponent that renderspicture,srcSet,sizes, intrinsic dimensions, lazy loading, and async decoding.npm start.This reduces image transfer and decode work on mobile while reserving image space to avoid layout shift.
Which issue(s) this PR fixes:
Fixes #662
AI assistance:
I used AI assistance to help me explore the codebase and review code. But solution was fully authored, manually reviewed, tested by me and take responsibility for all changes in this PR.
Checklist:
npm run lintandnpm run format:checkpassnpm run buildsucceeds for bothenandzhgit commit -s)Summary by CodeRabbit