Repository navigation
Fix cold-start file sharing to open Submit expense - #101208
nabi-ebrahimi wants to merge 18 commits into
Conversation
|
|
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ShareActionHandler.processFiles((processedFiles) => { | ||
| const tempFile = Array.isArray(processedFiles) ? processedFiles.at(0) : (JSON.parse(processedFiles) as ShareTempFile); | ||
| ShareActionHandler.processFiles((processedFiles: unknown) => { | ||
| let tempFile: ShareTempFile | undefined; |
There was a problem hiding this comment.
❌ PERF-2 (docs)
The callback runs the full parsing pipeline (array handling, isShareTempFile type guards, and a JSON.parse of the string branch) before checking if (errorTitle) { return; }. When errorTitle is already set, the result is discarded anyway, so all of that work is wasted. errorTitle is available up front, so this cheap guard should short-circuit before the expensive parsing.
Move the guard to the top of the callback:
ShareActionHandler.processFiles((processedFiles: unknown) => {
if (errorTitle) {
return;
}
let tempFile: ShareTempFile | undefined;
if (Array.isArray(processedFiles)) {
// ...parse...
}
// ...
});Reviewed at: 1ba8767 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
@dmkt9, Gentle bump. thanks. |
|
@dmkt9, Friendly bump. thanks. |
| object FileUtils { | ||
| private const val tag = "FileUtils" | ||
| private const val directoryName = "Expensify" | ||
| private const val shareDirectoryName = "sharedFiles" |
There was a problem hiding this comment.
Why do we need this changes?
There was a problem hiding this comment.
This directory is specifically used for temporary files received through the Android share intent, so renaming it to sharedFiles isolates these attachments from the app's general Expensify internal storage directory.
During testing, I verified that reusing the existing Expensify folder causes the shared attachment to fail validation (triggering the corrupted-attachment error) because temporary and general storage contents conflict when cleared. Using a dedicated sharedFiles directory ensures the attachment is cleanly copied, validated, and processed. Additionally, this aligns directly with the iOS share extension implementation, which already utilizes sharedFiles.
There was a problem hiding this comment.
I don't think this explanation matches the current implementation/history.
The existing Expensify directory appears to have been introduced specifically for Android share-intent temporary files, and I couldn't find any other Android code using that subdirectory as general app storage. clearTemporaryFiles() already clears that directory before copying the newly shared attachment into it, so it's unclear what "temporary and general storage contents" would conflict.
Also, the new implementation still clears the legacy Expensify directory in clearInternalStorageDirectory(), so if that directory actually contained general app data, those contents would still be deleted.
Using sharedFiles is reasonable for clearer semantics and consistency with iOS, but could you clarify the concrete cause of the validation failure you observed when using Expensify? For example, which other files were present there and how did they cause the newly copied attachment to fail validation?
| when(intent.action) { | ||
| Intent.ACTION_SEND -> { | ||
| handleSingleFileIntent(intent, context) | ||
| onCompleted() | ||
| super.clearTemporaryFiles(context) |
There was a problem hiding this comment.
If the action isn't Intent.ACTION_SEND, the temporary files aren't cleared, right?
There was a problem hiding this comment.
Good catch. I moved clearTemporaryFiles() prior to the intent.action evaluation to preserve the previous cleanup behavior across all intents handled by FileIntentHandler, including non-ACTION_SEND intents. This guarantees that stale temporary files are consistently cleared regardless of whether the incoming intent is ACTION_SEND, ACTION_SEND_MULTIPLE, or another unhandled action.
|
@MelvinBot review |
Code reviewThe approach is sound. Saving the shared file before React Native starts, then setting Should fix
Nits
I did not run a web test because this fix covers the Android share intent, which web can't exercise. |
Addressed the applicable feedback:
|
|
@dmkt9, thanks for your review! I’ve addressed all your feedback. Please take another look when you have a chance. |
| if (!destinationFile.exists() || destinationFile.length() == 0L) { | ||
| destinationFile.delete() | ||
| return null | ||
| } |
There was a problem hiding this comment.
Empty/corrupted files are silently dropped instead of displaying an attachment error
There was a problem hiding this comment.
Thank you for pointing this out. When saving an empty or corrupted shared attachment failed, the native handler returned false, which prevented the Share route from opening and caused the attachment to be silently discarded.
I have updated both the file and text share handlers to proceed to the Share route upon receiving an ACTION_SEND intent, even if attachment persistence fails. Because temporary share data is cleared beforehand, processFiles() resolves to null, allowing ShareRootPage to display the existing corrupted-attachment error UI. Valid attachments continue to follow the standard save-and-open flow.
| val mimeType = try { | ||
| context.contentResolver.getType(fileUri) | ||
| } catch (exception: Exception) { | ||
| null | ||
| } ?: intent.type |
There was a problem hiding this comment.
Why do we need to prioritize contentResolver.getType over intent.type? If contentResolver.getType resolves to the wrong type while intent.type is correct, we could incorrectly report a wrong file type error. Also, I haven't seen any bug reports related to intent.type.
There was a problem hiding this comment.
Thank you for pointing this out. The MIME-type precedence change is indeed not required for the cold-start fix.
I have updated both the file and text share handlers to prioritize intent.type whenever available, preserving the established behavior. contentResolver.getType() is now utilized strictly as a fallback for cases where the share intent does not provide a MIME type.
| val editor = sharedPreferences.edit() | ||
| editor.clear() | ||
| editor.apply() | ||
| editor.commit() |
There was a problem hiding this comment.
I was also quite concerned about this earlier. If the file is large enough, it could delay startup until we reach BootSplash
There was a problem hiding this comment.
Thank you for pointing this out. I have restored apply() for clearing the previous share data.
Since apply() updates the value in memory immediately for the current process, the cold-start share handoff remains intact while avoiding unnecessary synchronous disk I/O on the main thread prior to BootSplash initialization.
|
I see it on the latest |
|
@MelvinBot review |
Code review (re-review of
|
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx bun ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
|
This PR adds a new |
2dcd06b to
1b8bc19
Compare
justinpersaud
left a comment
justinpersaud
left a comment
There was a problem hiding this comment.
Bad merge? Why are there so many changes?
|
@dmkt9, Gentle bump. thanks. |
|
@nabi-ebrahimi Can you take a look at this comment? #101208 (comment) |
@dmkt9, You’re right—the please take another look when you get a chance thanks. |
| Intent.ACTION_SEND -> { | ||
| handleSingleFileIntent(intent, context) | ||
| onCompleted() | ||
| handleSingleFileIntent(intent, context, shouldLaunchActivity) |
There was a problem hiding this comment.
NAB since it's existing code: functions should be named after what they do, not what they handle.
There was a problem hiding this comment.
Thanks! I renamed the private helper methods to reflect what they do: saving the shared file or text data. No behavior was changed.
| handleSingleFileIntent(intent, context) | ||
| onCompleted() | ||
| handleSingleFileIntent(intent, context, shouldLaunchActivity) | ||
| // Open the Share screen even if persisting the attachment fails so it can display an error instead of silently discarding the share. |
There was a problem hiding this comment.
This comment makes me wonder if the handleSingleFileIntent call should be in a try block and onCompleted should be in a finally block. But then I look and see that that's not the behavior we have here - an exception thrown in handleSingleFileIntent would result in onCompleted not being called.
So, I suppose this comment left me with more questions than answers:
- How do we know if persisting the attachment failed?
- When is
shouldLaunchActivityfalse?
There was a problem hiding this comment.
Good questions!
- How do we know if persisting the attachment failed?
Expected persistence failures in saveSharedFile() / saveSharedText() return normally rather than throwing. For a warm start, execution then continues to onCompleted(), which opens the Share screen. Since no valid shared file was saved, the Share screen displays the attachment error instead of silently dropping the share. - When is shouldLaunchActivity false?
shouldLaunchActivity is false during a cold start. In that case, MainActivity processes the share intent before React Native initializes and sets new-expensify://share/root as the activity’s initial URL, rather than calling onCompleted().
I updated the inline comments inFileIntentHandler.ktandTextIntentHandler.ktto clarify this flow.
| } catch (e: Exception) { | ||
| null | ||
| } | ||
| val extension = if (mimeType != null) { |
There was a problem hiding this comment.
Here's a nice bit of Kotlin syntactic sugar you can use here:
val extension = mimeType?.let(mimeTypeMap::getExtensionFromMimeType) ?: "bin"There was a problem hiding this comment.
Nice suggestion, updated the MIME type extension fallback to use Kotlin’s null-safe let syntax. Behavior is unchanged.
| override fun onCreate(savedInstanceState: Bundle?) { | ||
| val initialIntent = intent | ||
| val isColdShareIntent = initialIntent?.action == Intent.ACTION_SEND | ||
| if (isColdShareIntent && initialIntent != null && handleIntent(initialIntent, shouldLaunchActivity = false)) { |
There was a problem hiding this comment.
I think this conditional can be simplified?
| if (isColdShareIntent && initialIntent != null && handleIntent(initialIntent, shouldLaunchActivity = false)) { | |
| if (isColdShareIntent && handleIntent(initialIntent, shouldLaunchActivity = false)) { |
There was a problem hiding this comment.
Good suggestion, removed the redundant null check. isColdShareIntent can only be true when initialIntent is an ACTION_SEND intent, so the behavior is unchanged.
|
@roryabraham, thanks for your review! I’ve addressed all your feedback. @dmkt9, could you please take a look at the latest changes when you have a chance? Thanks! |
This branch has not been deployed
5 participants
Explanation of Change
Fixed Android cold-start file sharing by persisting the incoming
ACTION_SENDattachment before React Native initializes. After persistence succeeds, the activity is initialized withnew-expensify://share/rootas its initial URL, rather than relying on a re-entrant deep link that can arrive before React Native is ready and be dropped.The native intent handlers now report whether attachment persistence succeeded and open the Share flow only after a successful handoff. The native share module safely handles missing, empty, and incomplete MIME data.
ShareRootPagenow safely handles native responses returned as an array, object, JSON string, ornull; normalizes MIME types; and shows an attachment error for invalid or unavailable files instead of leaving the Share flow indefinitely on its loading skeleton.Fixed Issues
$ #101194
PROPOSAL: N/A
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
I linked the correct issue in the
### Fixed Issuessection aboveI wrote clear testing steps that cover the changes made in this PR
TestssectionOffline stepssectionQA stepssectionI included screenshots or videos for tests on all platforms
I ran the tests on all platforms & verified they passed on:
I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
I followed proper code patterns (see Reviewing the code)
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I followed the guidelines as stated in the Review Guidelines
I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like
Avatar, I verified the components usingAvatarare working as expected)If a new CSS style is added I verified that:
StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))If new assets were added or existing ones were modified, I verified that:
npm run compress-svg)If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like
Avataris modified, I verified thatAvataris working as expected in all cases)If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
Designlabel and/or tagged@Expensify/designso the design team can review the changes.I asked an AI agent to review the changes for accessibility issues and addressed its findings.
I tested with a screen reader (VoiceOver on macOS) and verified all new/changed elements are reachable with a logical focus order.
I verified all new/changed elements have meaningful accessible names and roles.
I verified state changes are announced (e.g. checked/unchecked, expanded/collapsed, selected).
I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
If the
mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen_Recording_20260916_120030_New.Expensify.Dev.mp4
Android: mWeb Chrome
N/A
iOS: Native
Screen.Recording.2026-09-16.at.12.12.22.PM.mp4
iOS: mWeb Safari
N/A
MacOS: Chrome / Safari
N/A