Repository navigation
Read the picked file name from the URI when the native picker returns no metadata - #101356
Conversation
… no metadata On iOS the document picker can return a file with name, size and type all null when it fails to read the file's resource values. FilePicker replaced the missing name with the extensionless placeholder 'spreadsheet', so extension checks downstream rejected the file - .ofx and .qfx statement imports failed with 'Invalid file' on device. Recover the name from the URI, which still carries it, and keep the placeholder only when the URI has no extension either. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
To take ownership of the branch so you can push updates: Then you can close this PR and open a new one from your fork. |
|
@MelvinBot @codex review |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a4f885dbf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const fileName = getFileNameWithFallback(file.name, file.uri, DEFAULT_FILE_NAME); | ||
|
|
||
| if (!file.name) { | ||
| Log.warn('[FilePicker] The picker returned a file without metadata, so the file name was read from the URI', {error: file.error, fileName}); |
There was a problem hiding this comment.
Avoid uploading recovered bank-statement filenames
When this metadata-recovery path is exercised, fileName is user-controlled and may contain customer names or account identifiers. Log.warn is buffered through serverLoggingCallback in src/libs/Log.ts and uploaded to centralized backend logs, while this payload is not passed through sanitizeLogParams, so every affected selection discloses the recovered filename beyond the upload flow. Preserve the diagnostic signal using a non-sensitive status or error code, but omit or redact the filename.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Removed the Log.warn call and the now-unused Log import. Pushed in b40cf3e.
The fallback itself is unchanged — src/components/FilePicker/index.native.tsx:96 still reads the name from the URI, just silently. This also resolves the Codex concern above, since nothing uploads the recovered filename now.
Checks on this commit: typecheck passed, spell-changed passed, npm test tests/unit/FileUtilsTest.ts tests/unit/ImportSpreadsheetTest.ts passed (49 tests). ESLint and the React Compiler compliance check still report only the pre-existing Cannot access refs during render at line 151 — unchanged code, and it fails identically on origin/main on both compilers, so there is no regression and no new divergence.
| /** | ||
| * Returns the file name, recovering it from the URI when the name itself is missing. | ||
| * | ||
| * A file picker can hand back a file with no metadata at all - the iOS document picker does this whenever it fails to | ||
| * read the file's resource values, leaving `name`, `size` and `type` null. The URI still holds the real file name in | ||
| * that case, so take it from there rather than from a placeholder that carries no extension, which every extension | ||
| * check downstream would reject. `defaultFileName` is only used when the URI has no extension either, as is the case | ||
| * for the `content://` URIs Android returns. | ||
| */ |
There was a problem hiding this comment.
Cut it from 7 lines to 2 — kept only why the fallback exists and when the default is used, and dropped the walkthrough of the downstream extension checks. Pushed in 57d060a.
src/libs/fileDownload/FileUtils.ts:162-165:
/**
* Returns the file name, reading it from the URI when the picker returns none, as the iOS document picker can.
* Falls back to `defaultFileName` only when the URI has no extension either, as with Android's `content://` URIs.
*/ESLint on the file reports only the 4 pre-existing no-unsafe-type-assertion errors at lines 595, 705, 732 and 934 — none in the changed region, and unchanged from before this commit.
|
🤖 Android test steps verification results: ✅ pass The PR's Two things worth flagging that are outside the pass/fail: the PR's own bug is iOS-only, so Android exercises the unchanged branch of view run · no recording available |
Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
DetailsExplanation of ChangeOn iOS the document picker can return a picked file with no metadata at all —
The file name is still available — it is the last path segment of
The two other Fixed Issues$ #101281 Tests
Offline testsSame as Tests QA StepsSame as Tests
PR Author Checklist
@MelvinBot please set the PR body to the content in the details section above |
|
🤖 Done — the PR body is now the content you supplied, verbatim. Two things that changed as a side effect of replacing the whole body, in case either was unintentional:
Say the word if you'd like either restored. |
|
@MelvinBot remove Log.warn code. Not needed. |
Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
🤖 There's nothing for me to fix here — no code check is failing. Only two checks are red, and both need a human, not a commit:
Everything else passes: I didn't push anything, since no failure traces back to the diff. |
|
@aimane-chnaif Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
Done — the PR is out of draft and ready for review. The I re-ran the checks against that commit before marking it ready:
I didn't run a web pass for this change. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.MP4iOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
|
🚧 robertjchen has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/robertjchen in version: 9.4.81-0 🚀
|
|
🤖 Yes, help site changes are required. Draft PR: Docs updates for E/A#101356 document .ofx and .qfx personal card imports This PR itself is an internal bug fix — no new user-facing behavior. But it makes What I changed and whyThe live upload screen reads: "Drag and drop your spreadsheet or bank statement here, or choose a file below. Supported formats: .csv, .txt, .xls, .xlsx, .ofx, and .qfx." The article contradicted that.
No change needed for Every UI label was verified against the live NewDot web UI: the Wallet tab, the Cards section, Import transactions, Card display name, Next, Choose file, and the supported-formats line. Applied @aimane-chnaif, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.4.81-1 🚀
|
1 similar comment
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.4.81-1 🚀
|








Explanation of Change
On iOS the document picker can return a picked file with no metadata at all —
name,sizeandtypeare allnull, and onlyuri,nativeTypeand anerrorstring come back.@react-native-documents/pickerproduces this whenever its metadata read (url.resourceValues(...)) throws; on a physical device it throwsNSFileReadNoPermissionErrorfor the.ofx/.qfxfiles reported in the linked issue, which is why the simulator doesn't reproduce it.FilePickerreplaced that missing name with the literal'spreadsheet', which carries no extension. Every extension check downstream then failed:splitExtensionFromFileName('spreadsheet')returns'', soImportSpreadsheet.validateFilehit!allowedExtensions.includes('')and showed the "Invalid file type" modal instead of importing the statement.The file name is still available — it is the last path segment of
file.uri. This PR reads it from there when the picker gives us none:getFileNameWithFallback(fileName, uri, defaultFileName)toFileUtils. It returns the given name when there is one, otherwise the name embedded in the URI (via the existinggetFileName, so it is percent-decoded and sanitized), and only falls back to the default when the URI has no extension either. That guard matters on Android, wherecontent://URIs carry no file name.FilePicker/index.native.tsxfor both thekeepLocalCopyfile name and the returnedFileObject.name, so the cached copy keeps its real extension too instead of being written as an extensionlessspreadsheet.LocalCopy.nametostringnow thatpickFilealways resolves a name, which removes the duplicate fallback ingetDataForUpload.The two other
nullfields need no change:sizeis already recovered by the existingRNFetchBlob.fs.statcall, andtypeis unused byUploadOFX.Fixed Issues
$ #101281
PROPOSAL: #101281 (comment)
Tests
Offline tests
Same as Tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.