Fix SVG attachment sizing and preserve provided dimensions #6851
Conversation
WalkthroughImage dimension data is passed from the file object through the container to the MessageImage component. MessageImage now accepts optional provided dimensions, synchronizes them to state, applies SVG-specific sizing guards, computes display dimensions with fallbacks and minimums, and uses those dimensions in rendering and contentFit selection. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
app/containers/message/Components/Attachments/Image/Container.tsx(1 hunks)app/containers/message/Components/Attachments/Image/Image.tsx(2 hunks)app/containers/message/Components/Attachments/Image/definitions.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/containers/message/Components/Attachments/Image/Image.tsx (6)
app/containers/message/Components/Attachments/Image/definitions.ts (1)
IMessageImage(15-25)app/theme.tsx (1)
useTheme(29-29)app/lib/constants/keys.ts (1)
AUTOPLAY_GIFS_PREFERENCES_KEY(21-21)app/containers/message/Components/WidthAwareView.tsx (1)
WidthAwareContext(4-4)app/containers/UIKit/Image.tsx (1)
Image(47-47)app/lib/methods/helpers/isValidUrl.ts (1)
isValidUrl(3-9)
app/containers/message/Components/Attachments/Image/Container.tsx (1)
app/containers/message/Components/Attachments/Image/Image.tsx (1)
MessageImage(16-126)
🔇 Additional comments (8)
app/containers/message/Components/Attachments/Image/definitions.ts (1)
21-24: LGTM!The interface extension cleanly adds optional dimension metadata to support the SVG sizing fix. The doubly-optional structure (optional object with optional properties) is appropriate for handling cases where dimension data may not be available.
app/containers/message/Components/Attachments/Image/Container.tsx (1)
29-36: LGTM!The prop passing correctly threads image dimensions from the file metadata to the MessageImage component. The implementation is clean and preserves all existing props.
app/containers/message/Components/Attachments/Image/Image.tsx (6)
16-24: LGTM!The function signature correctly accepts the new
imageDimensionsprop with a clear alias toprovidedDimensions, and the state initialization properly defaults to zero dimensions when not provided.
30-34: LGTM!The effect correctly synchronizes the provided dimensions to local state with appropriate guards and dependency tracking.
36-55: LGTM! This is the core fix for SVG sizing.The early return for SVGs (lines 38-40) correctly prevents
Image.loadAsyncfrom overriding the provided/fallback dimensions with intrinsic SVG sizes. The promise handling with error logging is appropriate, and the dependency array correctly triggers re-runs when the image type, URI, or download status changes.
57-62: LGTM!The dimension calculation logic properly handles both the presence and absence of dimension data. It maintains aspect ratios, respects the maximum size constraint from context, and provides a sensible fallback size (200px) for images without dimension metadata.
68-73: LGTM!The container style correctly enforces a 64px minimum for each dimension, preventing collapse of small SVGs while using flexbox alignment to center the content. This ensures usability and adequate tap targets on mobile.
95-100: LGTM!The conditional
contentFitstrategy is well-chosen: 'contain' for SVGs preserves the complete graphic and aspect ratio, while 'cover' for bitmaps provides better aesthetic presentation. This aligns with the PR objective to improve SVG rendering.
| const maxSize = useContext(WidthAwareContext); | ||
| const showImage = isValidUrl(uri) && imageDimensions.width && status === 'downloaded'; | ||
| const isGif = imageType === 'image/gif'; | ||
| const isSvg = imageType?.includes('svg') ?? false; |
There was a problem hiding this comment.
Consider case-insensitive SVG detection.
The includes('svg') check is case-sensitive. While most servers return lowercase MIME types (e.g., 'image/svg+xml'), the HTTP specification doesn't mandate this. If a server returns 'image/SVG+xml', the SVG would not be detected and the sizing fix would fail.
Apply this diff for more robust detection:
- const isSvg = imageType?.includes('svg') ?? false;
+ const isSvg = imageType?.toLowerCase().includes('svg') ?? false;📝 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 isSvg = imageType?.includes('svg') ?? false; | |
| const isSvg = imageType?.toLowerCase().includes('svg') ?? false; |
🤖 Prompt for AI Agents
In app/containers/message/Components/Attachments/Image/Image.tsx around line 28,
the current SVG detection uses a case-sensitive includes('svg') which can miss
variants like 'image/SVG+xml'; update the check to be case-insensitive by first
confirming imageType exists and then calling
imageType.toLowerCase().includes('svg') (or use a safe conditional like
imageType ? imageType.toLowerCase().includes('svg') : false) so toLowerCase is
never called on undefined.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app/containers/message/Components/Attachments/Image/Image.tsx (4)
21-24: Consider validating provided dimensions.The initialization accepts any numeric value from
providedDimensionswithout verifying they are positive. If invalid dimensions (negative, zero, or non-finite numbers) are passed, they could propagate through the rendering logic.Apply this diff to add validation:
const [imageDimensions, setImageDimensions] = useState({ - width: providedDimensions?.width ?? 0, - height: providedDimensions?.height ?? 0 + width: (providedDimensions?.width && providedDimensions.width > 0) ? providedDimensions.width : 0, + height: (providedDimensions?.height && providedDimensions.height > 0) ? providedDimensions.height : 0 });
30-34: Validate dimensions before setting state.The condition
if (providedDimensions?.width && providedDimensions?.height)will reject zero values (which is correct) but will accept negative dimensions. Consider adding explicit positive checks for robustness.Apply this diff:
useEffect(() => { - if (providedDimensions?.width && providedDimensions?.height) { + if (providedDimensions?.width && providedDimensions.width > 0 && + providedDimensions?.height && providedDimensions.height > 0) { setImageDimensions({ width: providedDimensions.width, height: providedDimensions.height }); } }, [providedDimensions?.width, providedDimensions?.height]);
42-54: Consider cleanup for Image.loadAsync promise.If the component unmounts while
Image.loadAsyncis pending, the promise may still resolve and attempt to callsetImageDimensionson an unmounted component. While React 19 has improved handling for this, adding explicit cleanup would be more robust.Apply this diff to add cleanup:
useEffect(() => { // Preserve SVG sizing: stick to provided/fallback dimensions and skip intrinsic load that shrinks the view. if (isSvg) { return; } + let cancelled = false; if (status === 'downloaded') { Image.loadAsync(uri, { onError: e => { log(e); } }) .then(image => { - setImageDimensions({ width: image.width, height: image.height }); + if (!cancelled) { + setImageDimensions({ width: image.width, height: image.height }); + } }) .catch(e => { log(e); }); } + return () => { + cancelled = true; + }; }, [uri, status, isSvg]);
68-73: Consider extracting the minimum size constant.The minimum container size of 64px is hardcoded in two places. Extracting it to a named constant would improve maintainability and make the intent clearer.
Apply this diff:
+const MIN_CONTAINER_SIZE = 64; + const containerStyle: ViewStyle = { alignItems: 'center', justifyContent: 'center', - ...(displayWidth <= 64 && { width: 64 }), - ...(displayHeight <= 64 && { height: 64 }) + ...(displayWidth <= MIN_CONTAINER_SIZE && { width: MIN_CONTAINER_SIZE }), + ...(displayHeight <= MIN_CONTAINER_SIZE && { height: MIN_CONTAINER_SIZE }) };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/containers/message/Components/Attachments/Image/Image.tsx(2 hunks)
🔇 Additional comments (4)
app/containers/message/Components/Attachments/Image/Image.tsx (4)
28-28: Good fix for case-insensitive SVG detection.The case-insensitive check properly handles variations like 'image/SVG+xml' as recommended in previous reviews.
36-40: Excellent SVG sizing preservation.The early return prevents
Image.loadAsyncfrom overriding the provided/fallback dimensions for SVGs, which is the core fix for the reported issue where SVGs would shrink after the first render.
57-62: Solid dimension calculation logic.The display dimension calculations correctly:
- Preserve aspect ratios via the ratio calculation
- Bound dimensions to
maxSizeto prevent overflow- Provide sensible fallbacks when dimensions are unavailable
- Validate that final display dimensions are positive before showing the image
95-100: Appropriate contentFit selection.Using
'contain'for SVGs ensures the entire graphic is visible while preserving aspect ratio, and'cover'for raster images maintains the existing behavior. This differentiation is correct and aligns with the PR objectives.
1 participant
This change fixes an issue where SVG attachments were being resized incorrectly after the first render due to intrinsic sizing logic being re-applied... Causing the image not showing in channel
Closes #1466
What’s fixed
image_dimensions(or a consistent fallback) to keep the aspect ratio stable.contentFit="contain"for SVGs, while leaving GIF and bitmap handling unchanged.How It Works
Image.loadAsyncfor SVG files to avoid intrinsic SVG sizes overriding the intended layout.displayWidthanddisplayHeightfrom the provided dimensions (or a fallback) and maintain the correct aspect ratio.Visual Comparison
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.