fix: restore file existence check before image conversion #3
Conversation
- Add FileSystem.getInfoAsync check before ImageManipulator - Provides clear error message when file is missing/inaccessible - Fixes image upload failures on some devices Reverts part of 59f6976 that removed this validation
📝 WalkthroughWalkthroughRestores a missing file-existence check in the image converter and documents the fix. Adds an expo-file-system import and a pre-conversion check that throws if the input file is absent; includes a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🤖 Fix all issues with AI agents
In `@lib/upload/image-converter.ts`:
- Line 2: The import "import * as FileSystem from 'expo-file-system'" in
lib/upload/image-converter.ts requires adding expo-file-system to package.json
dependencies; add "expo-file-system" to the dependencies section (use the
matching version for your Expo SDK), run npm/yarn install to update node_modules
and the lockfile, and commit the updated package.json and
package-lock.json/yarn.lock so the runtime can resolve the FileSystem import.
🧹 Nitpick comments (3)
lib/upload/image-converter.ts (2)
44-48: Good restore of the file existence pre-check — but note the error is double-wrapped.The existence check itself is correct and addresses the root cause. However, the
throwon line 47 is inside thetryblock, so it will be caught by thecatchon line 67 and re-wrapped as:
Failed to convert image: Image file not found: <uri>This is still informative, but if the intent is to surface a distinct "file not found" error to callers (e.g., to show different UI), they won't be able to distinguish it from a conversion failure without parsing the message string.
Consider either re-throwing the file-not-found error before the
try, or checking for it in thecatchto avoid wrapping:Option: move the check before the try block
export async function convertToJPEG( uri: string, quality: number = 0.8 ): Promise<ConvertedImage> { + // Verify file exists before attempting conversion + const fileInfo = await FileSystem.getInfoAsync(uri); + if (!fileInfo.exists) { + throw new Error(`Image file not found: ${uri}`); + } + try { - // Verify file exists before attempting conversion - const fileInfo = await FileSystem.getInfoAsync(uri); - if (!fileInfo.exists) { - throw new Error(`Image file not found: ${uri}`); - } - // Convert to JPEG using ImageManipulatorNote: Moving the check outside
trymeans a failure ingetInfoAsyncitself (e.g., invalid URI scheme) would become an unhandled rejection. You'd need to decide which trade-off is preferable for your error-reporting strategy.
81-89:Promise.allwill fail fast on the first bad image — consider if that's desired.If one URI in the batch is invalid, the entire batch rejects immediately. If partial results are useful,
Promise.allSettledwould allow the caller to handle per-image failures. This is pre-existing behavior and not introduced by this PR, so just flagging for awareness.PR_DESCRIPTION.md (1)
1-27: Clear and well-structured PR documentation.One minor consideration: committing a
PR_DESCRIPTION.mdto the repo means it will persist after the PR is merged. If this is intended as living documentation (e.g., an ADR or changelog entry), that's fine. If it's only meant to accompany the PR, the content might be better suited to the PR body on GitHub to avoid accumulating stale markdown files over time.
| @@ -1,4 +1,5 @@ | |||
| import * as ImageManipulator from 'expo-image-manipulator'; | |||
| import * as FileSystem from 'expo-file-system'; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if expo-file-system is listed as a dependency in package.json
echo "=== Searching for package.json files ==="
find . -name 'package.json' -type f | head -5
echo ""
echo "=== Checking for expo-file-system in package.json ==="
fd -g 'package.json' --max-depth 3 --exec cat {} \; | grep -i 'expo-file-system' || echo "No match found"
echo ""
echo "=== Full dependencies section from package.json ==="
fd -g 'package.json' --max-depth 3 --exec jq '.dependencies, .devDependencies' {} \; 2>/dev/null || echo "Could not parse as JSON"Repository: SkateHive/mobileapp
Length of output: 1839
Add expo-file-system to package.json dependencies.
The code imports expo-file-system on line 2, but this package is not declared in package.json. This will cause a runtime failure. Add it to the dependencies section alongside the other expo packages.
🤖 Prompt for AI Agents
In `@lib/upload/image-converter.ts` at line 2, The import "import * as FileSystem
from 'expo-file-system'" in lib/upload/image-converter.ts requires adding
expo-file-system to package.json dependencies; add "expo-file-system" to the
dependencies section (use the matching version for your Expo SDK), run npm/yarn
install to update node_modules and the lockfile, and commit the updated
package.json and package-lock.json/yarn.lock so the runtime can resolve the
FileSystem import.
Addresses CodeRabbit review - prevents double-wrapping of error message
Re: expo-file-system dependency@coderabbitai This is a false positive. // lib/upload/image-upload.ts (line 1)
import { File } from 'expo-file-system';The package is available as a transitive dependency through the The import was also previously used in this same file before commit |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 10046
This is indeed a false positive. The ✏️ Learnings added
|
1 participant
Problem
Image uploads were failing on some devices with generic error messages. The issue was traced to commit 59f6976 which removed the file existence validation before attempting JPEG conversion.
Root Cause
Without the file existence check, if the image URI is invalid (permissions issue, cache cleared, etc.), the ImageManipulator fails with an unclear error instead of a helpful message.
Solution
Restored the file existence check in
image-converter.tsthat validates the file exists before attempting conversion:Testing
Related
Summary by CodeRabbit
Bug Fixes
Documentation