Conversation
* 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>
diegolmello
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
diegolmello
had a problem deploying
to
experimental_ios_build
GitHub Actions
Failure
diegolmello
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
diegolmello
temporarily deployed
to
official_android_build
GitHub Actions
Inactive
diegolmello
temporarily deployed
to
official_ios_build
GitHub Actions
Inactive
WalkthroughThe PR updates the application version from 4.66.0 to 4.66.1 across Android and iOS build configurations, and package.json. It also refactors the image URL handling in Urls.tsx by introducing a memoized getImageUrl helper function via useCallback and adjusting state typing and effect dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant Effect as Effect Hook
participant getImageUrl as getImageUrl (memoized)
participant API as axios
participant State as imageUrl State
Effect->>getImageUrl: compute URL from url.image or url.url
getImageUrl-->>Effect: return computed _imageUrl (string | null)
alt _imageUrl exists
Effect->>API: axios.head(_imageUrl)
API-->>Effect: response with headers
alt content-type is image
Effect->>State: setImageUrl(_imageUrl)
else content-type not image
Effect->>State: setImageUrl(null)
end
else _imageUrl is null
Effect->>State: setImageUrl(null)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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
🧹 Nitpick comments (4)
app/containers/message/Urls.tsx (4)
128-135: Make URL building robust (handle existing query, leading slashes, optional user).
Current concatenation can produce double slashes and breaks when _imageUrl already contains query params. It also assumes user is always present.Apply this diff to harden it:
-const getImageUrl = useCallback(() => { - const _imageUrl = url.image || url.url; - - if (!_imageUrl) return null; - if (_imageUrl.startsWith('http')) return _imageUrl; - return `${baseUrl}/${_imageUrl}?rc_uid=${user.id}&rc_token=${user.token}`; -}, [url.image, url.url, baseUrl, user.id, user.token]); +const getImageUrl = useCallback(() => { + const raw = url.image || url.url; + if (!raw) return null; + // Absolute external URL: return as-is + if (/^https?:\/\//i.test(raw)) return raw; + try { + const base = baseUrl.endsWith('/') ? baseUrl : `${baseUrl}/`; + const u = new URL(raw, base); + // Only append auth for same-origin URLs and when creds exist + if (user?.id && user?.token && u.origin === new URL(base).origin) { + u.searchParams.set('rc_uid', user.id); + u.searchParams.set('rc_token', user.token); + } + return u.toString(); + } catch { + return null; + } +}, [url.image, url.url, baseUrl, user?.id, user?.token]);
136-153: Avoid stale state; add cancellation and timeout to HEAD request.
- If the URL isn’t an image (or API_Embed false), imageUrl isn’t cleared—can render a stale preview.
- HEAD has no timeout/cancellation; can update state after unmount.
Apply this diff to improve resilience:
useEffect(() => { - const verifyUrlIsImage = async () => { - try { - const _imageUrl = getImageUrl(); - if (!_imageUrl || !API_Embed) return; - - const response = await axios.head(_imageUrl); - const contentType = response.headers['content-type']; - if (contentType?.startsWith?.('image/')) { - setImageUrl(_imageUrl); - } - } catch { - // do nothing - } - }; - verifyUrlIsImage(); -}, [url.image, url.url, API_Embed, getImageUrl]); + let cancelled = false; + const controller = new AbortController(); + const verifyUrlIsImage = async () => { + try { + const _imageUrl = getImageUrl(); + if (!_imageUrl || !API_Embed) { + if (!cancelled) setImageUrl(null); + return; + } + const response = await axios.head(_imageUrl, { signal: controller.signal, timeout: 5000 }); + const contentType = response.headers['content-type']; + if (!cancelled && contentType?.startsWith?.('image/')) { + setImageUrl(_imageUrl); + } else if (!cancelled) { + setImageUrl(null); + } + } catch { + if (!cancelled) setImageUrl(null); + } + }; + verifyUrlIsImage(); + return () => { + cancelled = true; + controller.abort(); + }; +}, [API_Embed, getImageUrl]);
153-153: Trim redundant deps.
url.image and url.url are already captured by getImageUrl; keep deps to [API_Embed, getImageUrl] to avoid duplicate runs.
71-82: Prevent state updates after unmount in UrlImage.
Guard Image.loadAsync with a cancellation flag; update state only if still mounted.useLayoutEffect(() => { - if (image && maxSize) { - Image.loadAsync(image, { - onError: () => { - setImageDimensions({ width: -1, height: -1 }); - }, - maxWidth: maxSize - }).then(image => { - setImageDimensions({ width: image.width, height: image.height }); - }); - } -}, [image, maxSize]); + let cancelled = false; + if (image && maxSize) { + Image.loadAsync(image, { + onError: () => { + if (!cancelled) setImageDimensions({ width: -1, height: -1 }); + }, + maxWidth: maxSize + }).then(img => { + if (!cancelled) setImageDimensions({ width: img.width, height: img.height }); + }); + } + return () => { + cancelled = true; + }; +}, [image, maxSize]);
📜 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 ignored due to path filters (1)
app/containers/message/__snapshots__/Message.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
android/app/build.gradle(1 hunks)app/containers/message/Urls.tsx(2 hunks)ios/RocketChatRN.xcodeproj/project.pbxproj(2 hunks)ios/RocketChatRN/Info.plist(1 hunks)ios/ShareRocketChatRN/Info.plist(1 hunks)package.json(1 hunks)
🔇 Additional comments (7)
package.json (1)
3-3: Version bump looks good and consistent.
Version set to 4.66.1. Matches platform files in this PR.ios/ShareRocketChatRN/Info.plist (1)
29-29: iOS Share extension marketing version updated correctly.
4.66.1 set on CFBundleShortVersionString.ios/RocketChatRN/Info.plist (1)
31-31: iOS app marketing version updated correctly.
CFBundleShortVersionString set to 4.66.1.ios/RocketChatRN.xcodeproj/project.pbxproj (1)
3037-3038: NotificationService MARKETING_VERSION aligned.
Both Debug/Release set to 4.66.1.Also applies to: 3082-3083
app/containers/message/Urls.tsx (2)
1-1: Import addition is appropriate.
useCallback import matches new helper usage.
126-127: State typing improvement LGTM.
imageUrl as string | null is clearer and safer for rendering checks.android/app/build.gradle (1)
93-93: Android versionName aligned.
versionName set to "4.66.1". Verify VERSIONCODE is appropriately bumped in CI/properties for Play uploads.Run to confirm where VERSIONCODE is defined and its current value:
diegolmello
temporarily deployed
to
upload_official_android
GitHub Actions
Inactive
|
Android Build Available Rocket.Chat 4.66.1.107670 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNTXEWev2uj8vGT2qSDy29vEhWPhYMnOk5RwWSZCFEeGLi-sZTULf6nc_Xh7mGnGuOM58uAAILaDYasT1faH |
|
Android Build Available Rocket.Chat 4.66.1.107670 |
diegolmello
temporarily deployed
to
upload_official_ios
GitHub Actions
Inactive
|
iOS Build Available Rocket.Chat 4.66.1.107672 |
diegolmello
had a problem deploying
to
upload_experimental_android
GitHub Actions
Failure
|
Android Build Available Rocket.Chat Experimental 4.66.1.107674 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNSgoO0JnapX9m22FKRJ4OelQIG8QlNOoknm05q6npesoTqMyDn-ga_Cs1TeL6wKHJbjjtejPI21zVVl80fv |
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Failure
diegolmello
had a problem deploying
to
experimental_android_build
GitHub Actions
Failure
diegolmello
had a problem deploying
to
experimental_ios_build
GitHub Actions
Failure
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Failure
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Failure
1 participant
Proposed changes
Issue(s)
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
Bug Fixes
Chores