feat: implement event cover photo picker, resumable upload progress, and native caching#425
feat: implement event cover photo picker, resumable upload progress, and native caching#425Nicks-19 wants to merge 1 commit into
Conversation
…ogress bar, and caching for event cover photos
📝 WalkthroughWalkthroughThe PR adds resumable image uploads with client-side validation and real-time progress feedback to event creation, while enforcing consistent force-cache directives for banner images across event display screens. CreateEvent now validates image size (≤ 5MB), compresses quality, uploads via Firebase resumable upload, and displays a progress overlay; event creation fails gracefully if upload errors occur. Four screens now use force-cache for banner images. ChangesImage Upload and Caching Enhancements
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/components/EventCard.js (1)
355-360:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd platform handling for
Imagesource.cache: 'force-cache'(iOS-documented; Android/Web parity isn’t guaranteed)
cache: 'force-cache'is set unconditionally inapp/src/components/EventCard.js,app/src/screens/RemindersScreen.js, andapp/src/screens/EventDetail.js; React Native docs describe these cache policies under iOS, so Android/Web behavior can be inconsistent/ignored.- Suggested fix: only pass
cacheon platforms you’ve validated, or switch to a cross-platform caching approach (there’s noreact-native-fast-image/expo-image-style dependency in this repo right now).<Image source={{ uri: event.bannerUrl || 'https://dummyimage.com/800x400/cccccc/000000.png&text=No+Image', ...(Platform.OS === 'ios' && { cache: 'force-cache' }), }} /* ... */ />Using
force-cachecan also serve stale banners even after updates.🤖 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 `@app/src/components/EventCard.js` around lines 355 - 360, EventCard.js unconditionally sets Image source.cache to 'force-cache', which is iOS-specific; update the Image source construction in the EventCard component to only include cache when Platform.OS === 'ios' (import Platform from 'react-native') or remove cache entirely and switch to a cross-platform caching strategy; specifically modify the source object built for the Image that uses event.bannerUrl (and the fallback URL) so cache is conditionally spread only for iOS to avoid inconsistent behavior on Android/Web and stale images.
♻️ Duplicate comments (2)
app/src/screens/RemindersScreen.js (1)
219-224:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPlatform compatibility issue:
cacheproperty is iOS-only.This is the same iOS-only limitation affecting EventCard.js and EventDetail.js. Consider applying a consistent platform-specific approach across all three files to ensure uniform caching behavior.
🤖 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 `@app/src/screens/RemindersScreen.js` around lines 219 - 224, The Image usage in RemindersScreen.js sets the iOS-only cache prop on the <Image> element (source object with cache: 'force-cache'), causing platform incompatibility; update the image source handling in the component that renders item.bannerUrl (used with styles.cardImage) to apply caching only on iOS (e.g., wrap the source prop creation with Platform.OS === 'ios' or use Platform.select) or switch to a cross-platform image solution (like react-native-fast-image) and mirror the same approach you used for EventCard.js and EventDetail.js so all three components use the same platform-safe caching logic.app/src/screens/EventDetail.js (1)
1368-1375:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPlatform compatibility issue:
cacheproperty is iOS-only.The
cacheproperty is only supported on iOS. This creates inconsistent behavior where iOS users get forced caching while Android and Web users don't. The same fix as suggested for EventCard.js applies here - use Platform.OS conditional or a cross-platform caching solution.🤖 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 `@app/src/screens/EventDetail.js` around lines 1368 - 1375, The ImageBackground usage in EventDetail.js sets a platform-specific `cache` prop (inside the source object) which only works on iOS; update the ImageBackground `source` construction (the object built with `uri: event.bannerUrl || 'https://dummyimage.com/...'`) to include `cache: 'force-cache'` only when Platform.OS === 'ios' (import Platform from 'react-native') or switch to a cross-platform caching solution used elsewhere (same approach as fixed in EventCard.js) so Android/Web consumers don't receive an unsupported prop; keep the rest (styles.headerImage, event.bannerUrl fallback) unchanged.
🧹 Nitpick comments (1)
app/src/screens/CreateEvent.js (1)
158-191: 💤 Low valueConsider deduplicating the 5MB validation shared with
pickImage.The
fetch(uri) → blob() → blob.size > 5 * 1024 * 1024check and the5 * 1024 * 1024literal are repeated in bothpickImageanduploadImage. Extracting a shared constant (and optionally a small helper) keeps the limit consistent if it ever changes.♻️ Suggested extraction
+const MAX_IMAGE_BYTES = 5 * 1024 * 1024; + const DEFAULT_BANNERS = [- if (blob.size > 5 * 1024 * 1024) { + if (blob.size > MAX_IMAGE_BYTES) { throw new Error('Image size exceeds 5MB limit.'); }The resumable upload, progress wiring, and error/completion handling otherwise look correct.
🤖 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 `@app/src/screens/CreateEvent.js` around lines 158 - 191, The image size check (fetch(...).blob().size > 5 * 1024 * 1024) is duplicated between pickImage and uploadImage; extract a shared constant (e.g., MAX_IMAGE_BYTES) and optionally a small helper function (e.g., isImageTooLarge(blob) or validateImageSize(blob)) and replace the literal in both pickImage and uploadImage to use the constant/helper so the limit stays consistent and changes only in one place.
🤖 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.
Outside diff comments:
In `@app/src/components/EventCard.js`:
- Around line 355-360: EventCard.js unconditionally sets Image source.cache to
'force-cache', which is iOS-specific; update the Image source construction in
the EventCard component to only include cache when Platform.OS === 'ios' (import
Platform from 'react-native') or remove cache entirely and switch to a
cross-platform caching strategy; specifically modify the source object built for
the Image that uses event.bannerUrl (and the fallback URL) so cache is
conditionally spread only for iOS to avoid inconsistent behavior on Android/Web
and stale images.
---
Duplicate comments:
In `@app/src/screens/EventDetail.js`:
- Around line 1368-1375: The ImageBackground usage in EventDetail.js sets a
platform-specific `cache` prop (inside the source object) which only works on
iOS; update the ImageBackground `source` construction (the object built with
`uri: event.bannerUrl || 'https://dummyimage.com/...'`) to include `cache:
'force-cache'` only when Platform.OS === 'ios' (import Platform from
'react-native') or switch to a cross-platform caching solution used elsewhere
(same approach as fixed in EventCard.js) so Android/Web consumers don't receive
an unsupported prop; keep the rest (styles.headerImage, event.bannerUrl
fallback) unchanged.
In `@app/src/screens/RemindersScreen.js`:
- Around line 219-224: The Image usage in RemindersScreen.js sets the iOS-only
cache prop on the <Image> element (source object with cache: 'force-cache'),
causing platform incompatibility; update the image source handling in the
component that renders item.bannerUrl (used with styles.cardImage) to apply
caching only on iOS (e.g., wrap the source prop creation with Platform.OS ===
'ios' or use Platform.select) or switch to a cross-platform image solution (like
react-native-fast-image) and mirror the same approach you used for EventCard.js
and EventDetail.js so all three components use the same platform-safe caching
logic.
---
Nitpick comments:
In `@app/src/screens/CreateEvent.js`:
- Around line 158-191: The image size check (fetch(...).blob().size > 5 * 1024 *
1024) is duplicated between pickImage and uploadImage; extract a shared constant
(e.g., MAX_IMAGE_BYTES) and optionally a small helper function (e.g.,
isImageTooLarge(blob) or validateImageSize(blob)) and replace the literal in
both pickImage and uploadImage to use the constant/helper so the limit stays
consistent and changes only in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2b394fc0-6e9b-4b11-ab3a-e956e045e6f6
📒 Files selected for processing (4)
app/src/components/EventCard.jsapp/src/screens/CreateEvent.jsapp/src/screens/EventDetail.jsapp/src/screens/RemindersScreen.js
|
@Nicks-19 fix that one issue flagged by sonar keep it 0 |
|
fix this Size check in pickImage fetches the blob just to check size, then fetches it again in uploadImage |
|
force-cache on EventCard and EventDetail banners means updated banners won't show |
2 participants
Description
This change fixes the problem with custom event cover photos in the Uni-Event application that was reported in issue #322.
Key Changes
Image Picker & Compression: We use
expo-image-pickerto let users pick images and we compress these images to make them smaller the quality is set to0.7.Validation: We check the size of the image before it is uploaded if the image is than
5MBwe do not let the user upload it.Upload: We use
uploadBytesResumablefrom Firebase Storage so we can see how the upload is going in real time.Upload Progress Overlay: When a user is uploading a cover photo we show an overlay on top of the photo slot this overlay is see through and it shows a progress bar and the percentage of the upload that is complete.
Native Caching: We use cache:
force-cacheon all the places where we show cover photos, likeEventCard,EventDetail, andRemindersScreen. The photos load faster.Verification Results
Summary by CodeRabbit
New Features
Performance