fix(image): added optional disable optimization #1479
Conversation
Tagging OptionsShould a new tag be published when this PR is merged?
|
WalkthroughAdds a new boolean flag Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Picture
participant Image
participant Source
participant Utils as getSrcSet / getOptimizedMediaUrl
Note over Picture,Image: Prop flow includes `disableims?: boolean`
Picture->>Source: render (passes disableims via Context/props)
Source->>Image: render <disableims>
Image->>Utils: getSrcSet(src, sizes, ..., disableims)
Utils->>Utils: build optimized URL (append ?disableims=true if set)
Utils-->>Image: srcSet (with URLs reflecting disableims)
Image-->>Browser: render <img src/srcSet> and possibly preload/link hints
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
website/components/Image.tsx (1)
107-148: Backend does not handle thedisableimsparameter—the feature is non-functional.The frontend correctly appends
disableims=trueto the URL (line 148), but the backend loader:
- Props type (website/utils/image/engine.ts) does not include a field for this parameter
- parseParams function (website/loaders/image.ts) only extracts
src,width,height,fit,quality—disableimsis silently ignoredThis means the
disableImageOptimizationflag has no effect; all images are always optimized regardless of this setting.Required fixes:
- Add
disableImageOptimization?: booleanto the Props interface- Extract and pass this parameter through parseParams
- Implement logic in the image engines to respect this flag
🧹 Nitpick comments (2)
website/components/Picture.tsx (2)
13-21: Consider adding JSDoc documentation for consistency.The
disableImageOptimizationfield is missing a JSDoc comment. While the name is self-explanatory, adding documentation would be consistent with the prop definitions in other parts of the codebase.Apply this diff to add documentation:
interface Context { preload?: boolean; + /** @description Disable image optimization */ disableImageOptimization?: boolean; }
101-105: Consider adding JSDoc documentation.Similar to the Context interface, consider adding a JSDoc comment to document the
disableImageOptimizationprop, consistent with how it's documented inImage.tsx(line 29).Apply this diff:
type Props = Omit<JSX.IntrinsicElements["picture"], "preload"> & { children: ComponentChildren; preload?: boolean; + /** @description Disable image optimization */ disableImageOptimization?: boolean; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
website/components/Image.tsx(10 hunks)website/components/Picture.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
website/components/Picture.tsx (1)
website/components/Image.tsx (2)
SetEarlyHint(11-11)Props(12-32)
🔇 Additional comments (6)
website/components/Image.tsx (4)
29-30: LGTM: Clear prop definition.The new prop is properly typed and documented, consistent with other prop definitions in the interface.
59-59: LGTM: Consistent type definition.The field is properly added to the optimization options interface.
167-197: LGTM: Proper parameter propagation.The
disableImageOptimizationparameter is correctly added to the function signature and properly passed togetOptimizedMediaUrl.
199-227: LGTM: Early hints respect the disable flag.The
disableImageOptimizationparameter is properly added to the function signature and correctly propagated togetOptimizedMediaUrl, ensuring early hints also respect the optimization flag.website/components/Picture.tsx (2)
39-74: LGTM: Proper Context usage and propagation.The
disableImageOptimizationflag is correctly read from Context and properly passed to bothgetSrcSetandgetEarlyHintFromSrcProps, ensuring consistent behavior throughout the Source component.
107-122: LGTM: Correct useMemo implementation.The
disableImageOptimizationprop is properly destructured, included in the memoized value, and correctly added to the dependency array. This ensures the Context value updates appropriately when the prop changes.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
website/loaders/image.ts (1)
23-37: All four image engines must be updated to handle thedisableimsflag.Verification confirms the original concern: while
disableimsis now parsed and included in the Params interface, none of the engine implementations (cloudflare, wasm, deco, passThrough) actually use it. The parameter is propagated toengine.resolve()but never acted upon, meaning the feature to skip optimization whendisableims: trueis set won't work.
- Cloudflare engine (
website/utils/image/engines/cloudflare/engine.ts): Destructures params but doesn't checkdisableims- Wasm engine (
website/utils/image/engines/wasm/engine.ts): Destructures params but doesn't checkdisableims- Deco engine (
website/utils/image/engines/deco/engine.ts):urlFromParamsdoesn't checkdisableims- PassThrough engine (
website/utils/image/engines/passThrough/engine.ts): Only destructuressrc, completely ignoresdisableimsEach engine needs to check the
disableimsflag and skip its optimization logic when true.
🧹 Nitpick comments (1)
website/loaders/image.ts (1)
36-36: Simplify the boolean conversion.The ternary
disableims ? true : falseis redundant and can be simplified.Apply this diff:
- disableims: disableims ? true : false, + disableims: Boolean(disableims),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
website/components/Image.tsx(10 hunks)website/components/Picture.tsx(5 hunks)website/loaders/image.ts(2 hunks)website/utils/image/engine.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- website/components/Picture.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
website/loaders/image.ts (1)
website/components/Image.tsx (1)
Props(12-32)
🔇 Additional comments (6)
website/utils/image/engine.ts (1)
7-7: LGTM! Clean API extension.The optional
disableimsproperty is properly typed and extends the public API surface for all engine implementations.website/components/Image.tsx (5)
29-30: LGTM! Clear documentation added.The
disableimsproperty is properly documented and typed as an optional boolean.
107-165: LGTM! Proper propagation through optimization pipeline.The
disableimsflag is correctly:
- Destructured from options (line 108)
- Added to query parameters when truthy (line 148)
- Propagated to both the Azion and standard optimization URL paths
This ensures the flag reaches the image loader for downstream handling.
167-197: LGTM! Consistent parameter threading.The
disableimsparameter is properly added togetSrcSetsignature and correctly forwarded togetOptimizedMediaUrlfor each factor in the srcset generation.
199-227: LGTM! Early hints support the new flag.The
disableimsparameter is correctly added togetEarlyHintFromSrcPropsand properly propagated togetOptimizedMediaUrl, ensuring preload hints respect the optimization flag.
229-295: LGTM! Component properly wires the flag.The Image component correctly propagates
props.disableimsto both:
getSrcSetfor srcset generation (line 246)getEarlyHintFromSrcPropsfor preload hints (line 272)This ensures the flag flows through all optimization paths.
What is this Contribution About?
Please provide a brief description of the changes or enhancements you are proposing in this pull request.
Issue Link
Please link to the relevant issue that this pull request addresses:
Loom Video
Demonstration Link
Summary by CodeRabbit