fix(catalog): send browser headers for image requests on all platforms#1896
fix(catalog): send browser headers for image requests on all platforms#1896
Conversation
Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
|
@coderabbitai Please review this PR for code quality, best practices, and potential issues. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainThe PR description images show iOS catalog search results demonstrating the bug and fix:
Let me fetch the file to do a thorough review. 🏁 Script executed: Length of output: 1619 🏁 Script executed: Length of output: 8508
✅ Core Fix — CorrectRemoving the
|
| Severity | Notes | |
|---|---|---|
fetchImageExtension missing UA headers |
Medium | May cause wrong file extension in cache for UA-gated CDNs |
| Duplicate UA string | Low | Maintenance risk |
No onError fallback |
Low | Silent failure UX |
The core fix is solid. Issues #2 and #3 would be good to address in this PR; issue #1 could be a follow-up since it affects a separate code path (cacheCatalogItemImage), not the display path fixed here.
|
@copilot CodeRabbit has reviewed this PR and provided feedback. Please address the review comments and push updates. Once you've made the changes, the monitor will ask CodeRabbit to re-review. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe change removes platform-specific conditional logic from the CatalogItemImage component's Image source configuration. User-Agent and Accept headers are now always included, replacing the previous Android-only branch to enable image loading on iOS. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 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)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Fixes inconsistent catalog image loading on iOS by ensuring remote image requests always include browser-like headers that certain CDNs require to return the real asset instead of a placeholder.
Changes:
- Removed the Android-only guard so
CatalogItemImagesendsUser-AgentandAcceptheaders on all platforms. - Dropped the now-unused
Platformimport.
You can also share your feedback on Copilot code review. Take the survey.
| headers: { | ||
| 'User-Agent': | ||
| 'Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/135.0.0.0 Safari/537.36', | ||
| Accept: 'image/webp,image/apng,image/*,*/*;q=0.8', | ||
| }, |
There was a problem hiding this comment.
The User-Agent/Accept header block is duplicated elsewhere (e.g. apps/expo/lib/utils/ImageCacheManager.ts). Consider centralizing these “browser-like” image request headers into a shared constant/helper so they don’t drift over time and can be updated in one place if CDN requirements change.
…8-4c37-8a20-0b5434a547d1
Deploying packrat-landing with
|
| Latest commit: |
6b0a115
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b999bd7a.packrat-landing.pages.dev |
| Branch Preview URL: | https://copilot-fix-181429480-611845.packrat-landing.pages.dev |
…8-4c37-8a20-0b5434a547d1
Isthisanmol
left a comment
There was a problem hiding this comment.
Don't think that it is a platform specific issue. As it does not fixes the issue #1848
for some of the categories on both the platform.
Catalog search results showed placeholder images on iOS because
CatalogItemImagewas only sendingUser-AgentandAcceptheaders on Android. Several image CDNs (Mackage, Black Diamond, etc.) gate image serving on a recognized browser UA — requests without these headers silently fall back to placeholders.Changes
CatalogItemImage.tsx— removedPlatform.OS === 'android'guard; headers are now sent unconditionally on all platformsPlatformimportThis explains the per-brand inconsistency: CDNs with stricter UA checks (Mackage, Black Diamond) failed on iOS while more permissive ones (Patagonia) worked fine.
Original prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.
Summary by CodeRabbit