fix: image preview crashing on large pdfs#6763
Conversation
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
WalkthroughIntroduces a memoized Changes
Sequence Diagram(s)sequenceDiagram
participant Component as Urls.tsx
participant Helper as getImageUrl()
participant HTTP as axios.head()
participant State as setImageUrl()
Component->>Helper: call getImageUrl()
Helper-->>Component: returns _imageUrl or null
alt _imageUrl present
Component->>HTTP: HEAD _imageUrl
HTTP-->>Component: response (content-type / status)
alt response is image AND API_Embed enabled
Component->>State: setImageUrl(_imageUrl)
else
Component->>State: setImageUrl(null)
end
else _imageUrl null
Component->>State: setImageUrl(null)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
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 (1)
app/containers/message/Urls.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🔇 Additional comments (3)
app/containers/message/Urls.tsx (3)
1-1: LGTM! Correct import for memoization.The addition of
useCallbackis appropriate for the memoizedgetImageUrlfunction introduced later in the component.
126-126: LGTM! Improved type safety.Changing
imageUrlto nullable and initializing tonullclearly expresses that the image URL is unavailable until verified, improving type safety and intent.
128-134: LGTM! Proper memoization.The memoized
getImageUrlfunction correctly computes the image URL with appropriate dependencies, preventing unnecessary recreations on each render.
| useEffect(() => { | ||
| const verifyUrlIsImage = async () => { | ||
| try { | ||
| const imageUrl = getImageUrl(); | ||
| if (!imageUrl || !API_Embed) return; | ||
| const _imageUrl = getImageUrl(); | ||
| if (!_imageUrl || !API_Embed) return; | ||
|
|
||
| const response = await axios.head(imageUrl); | ||
| const response = await axios.head(_imageUrl); | ||
| const contentType = response.headers['content-type']; | ||
| if (contentType?.startsWith?.('image/')) { | ||
| setImageUrl(imageUrl); | ||
| setImageUrl(_imageUrl); | ||
| } | ||
| } catch { | ||
| // do nothing | ||
| } | ||
| }; | ||
| verifyUrlIsImage(); | ||
| }, [url.image, url.url, API_Embed]); | ||
|
|
||
| const getImageUrl = () => { | ||
| const _imageUrl = url.image || url.url; | ||
|
|
||
| if (!_imageUrl) return null; | ||
| if (_imageUrl.includes('http')) return _imageUrl; | ||
| return `${baseUrl}/${_imageUrl}?rc_uid=${user.id}&rc_token=${user.token}`; | ||
| }; | ||
| }, [url.image, url.url, API_Embed, getImageUrl]); |
There was a problem hiding this comment.
Add timeout and cleanup to prevent hangs and memory leaks.
The verification approach is sound and likely fixes the PDF crash issue by checking content-type before rendering. However, two concerns:
- No request timeout: The
axios.headcall could hang indefinitely on slow or unresponsive servers - No cleanup: If the component unmounts during the request,
setImageUrlwill be called on an unmounted component, causing React warnings
Apply this diff to add timeout and cleanup:
useEffect(() => {
+ let isMounted = true;
const verifyUrlIsImage = async () => {
try {
const _imageUrl = getImageUrl();
if (!_imageUrl || !API_Embed) return;
- const response = await axios.head(_imageUrl);
+ const response = await axios.head(_imageUrl, { timeout: 5000 });
const contentType = response.headers['content-type'];
- if (contentType?.startsWith?.('image/')) {
+ if (isMounted && contentType?.startsWith?.('image/')) {
setImageUrl(_imageUrl);
}
} catch {
// do nothing
}
};
verifyUrlIsImage();
+ return () => {
+ isMounted = false;
+ };
}, [url.image, url.url, API_Embed, getImageUrl]);
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
diegolmello
force-pushed
the
fix.url-image-large-pdf
branch
from
1e1fecf to
656dfaa
Compare
diegolmello
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
diegolmello
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Error
diegolmello
temporarily deployed
to
experimental_ios_build
GitHub Actions
Inactive
OtavioStasiak
left a comment
OtavioStasiak
left a comment
There was a problem hiding this comment.
Looks good to me!
PS: I agree with @nmagedman’s suggestion to use startsWith instead of includes.
https://github.com/RocketChat/Rocket.Chat.ReactNative/pull/6763/files#r2498967354
Co-authored-by: Noach Magedman <nmagedman@gmail.com>
|
Thanks for your review, @nmagedman! |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/containers/message/Urls.tsx (1)
136-151: Add timeout and cleanup to prevent hangs and memory leaks.The content-type verification correctly prevents PDFs from being rendered as images (fixing the crash), but the implementation still needs timeout and cleanup handling as flagged in the previous review.
🧹 Nitpick comments (1)
app/containers/message/Urls.tsx (1)
152-152: Consider simplifying redundant dependencies.The effect dependencies include both
url.imageandurl.urlexplicitly, as well asgetImageUrlwhich already depends on those values. Whenurl.imageorurl.urlchanges,getImageUrlis recreated (per its dependency array), which already triggers this effect.Apply this diff to remove the redundant dependencies:
- }, [url.image, url.url, API_Embed, getImageUrl]); + }, [API_Embed, getImageUrl]);
📜 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/Urls.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: format
🔇 Additional comments (3)
app/containers/message/Urls.tsx (3)
1-1: LGTM!The
useCallbackimport is necessary for the new memoizedgetImageUrlhelper and follows React best practices.
126-126: LGTM!The type change to
string | nullwith null initialization correctly reflects that the image URL is unknown until verified by the HEAD request.
128-134: LGTM!The memoized helper correctly computes the image URL with proper handling for absolute URLs and credential injection. The dependency array is complete and accurate.
|
iOS Build Available Rocket.Chat Experimental 4.67.0.107662 |
diegolmello
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
diegolmello
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
diegolmello
temporarily deployed
to
official_ios_build
GitHub Actions
Inactive
diegolmello
temporarily deployed
to
official_android_build
GitHub Actions
Inactive
diegolmello
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
* Fix image preview crashing on large pdfs * Update snapshots * Update app/containers/message/Urls.tsx Co-authored-by: Noach Magedman <nmagedman@gmail.com> --------- Co-authored-by: Noach Magedman <nmagedman@gmail.com>
|
I'm just recording here that among all the misc. code refactorings/tweaks, the actual bugfix itself is this: The |
diegolmello
temporarily deployed
to
upload_official_android
GitHub Actions
Inactive
|
Android Build Available Rocket.Chat 4.67.0.107664 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNRDDVorsSTClD86-0klNWT89uLqO5oIKV7pGrC6BKGS7A7yI2BvIce4OUGhldVmvtB5JtUhOBx1rNhiF_F3 |
|
Android Build Available Rocket.Chat 4.67.0.107664 |
3 participants
Proposed changes
Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1075
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
Bug Fixes
Performance