Repository navigation
Migrate ShareActionHandlerModule to a type-safe Nitro module - #104550
nabi-ebrahimi wants to merge 2 commits into
Conversation
Generate one shared-file contract for Swift, Kotlin, and TypeScript. Keep native storage internal and preserve the existing Onyx timestamp format. Refs: Expensify#104432
|
|
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@shubham1206agra 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] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }; | ||
|
|
||
| interface ShareActionHandler extends HybridObject<{ios: 'swift'; android: 'kotlin'}> { | ||
| /** Returns an empty array when no share is pending; rejects if pending data cannot be read. */ |
There was a problem hiding this comment.
❌ CONSISTENCY-15 (docs)
This newly added JSDoc comment joins two independent clauses with a semicolon (...when no share is pending; rejects if pending data cannot be read.). Comments should read as plain, natural sentences; a semicolon used to splice two statements together should be split into two separate sentences.
Suggested fix:
/** Returns an empty array when no share is pending. Rejects if pending data cannot be read. */
processFiles(): Promise<SharedFile[]>;Reviewed at: e0ee644 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| return [] | ||
| } | ||
|
|
||
| return try files.map { try self.processFile($0) } |
There was a problem hiding this comment.
(Shubham's Ai) This rejects the whole batch if any single file fails processFile (empty, unreadable, not a regular file, or non-UTF-8 text_to_read). The old ObjC module skipped bad files and returned the rest. Could we keep that behavior, e.g. compactMap with try? (logging the skipped file), and only reject if no usable file remains?
| const tempFile = processedFiles.at(0); | ||
| if (!tempFile) { | ||
| setErrorTitle(translate('attachmentPicker.attachmentError')); | ||
| setErrorMessage(translate('attachmentPicker.errorWhileSelectingCorruptedAttachment')); |
There was a problem hiding this comment.
(Shubham's Ai) An empty result now shows errorWhileSelectingCorruptedAttachment, but nothing is corrupt in this case; there's just no pending share. This path also runs whenever the app returns to the foreground (the AppState listener calls handleProcessFiles again). Is there an existing message that fits "nothing to share" better?
| return@parallel arrayOf(SharedFile("text", content, "txt", timestamp, null)) | ||
| } | ||
|
|
||
| if (!file.isFile || !file.canRead() || file.length() == 0L) { |
There was a problem hiding this comment.
(Shubham's Ai) Throwing on a zero-byte file (here and the size > 0 guard in the Swift implementation) means the user gets the generic attachment error instead of the existing "attachment too small" (sizeNotMet) message from ShareRootPage. Could we let empty files through and keep the JS size validation as the single source of that error?
| } | ||
|
|
||
| const {fileExtension} = splitExtensionFromFileName(tempFile?.content); | ||
| if (tempFile) { |
There was a problem hiding this comment.
(Shubham's Ai) Nit: after the if (!tempFile) return guard above, this if (tempFile) is always true and the tempFile?. optional chains in this block are redundant. This could be flattened.
| const handleProcessFiles = () => { | ||
| ShareActionHandler.processFiles() | ||
| .then((processedFiles) => { | ||
| if (errorTitle) { |
There was a problem hiding this comment.
(Shubham's Ai) errorTitle here is the value from the render that started the call, not the current one, so it is stale by the time the promise resolves. This existed before, but the async chain widens the gap. Consider a ref or at least a comment.
|
|
||
| override fun processFiles(): Promise<Array<SharedFile>> = Promise.parallel { | ||
| val context = NitroModules.applicationContext ?: throw IOException("React Native context is unavailable") | ||
| val preferences = context.getSharedPreferences("shareActionHandler", Context.MODE_PRIVATE) |
There was a problem hiding this comment.
(Shubham's Ai) Nit: "shareActionHandler" / "shareObject" duplicate IntentHandlerConstants (in both App and Mobile-Expensify), which this module can't import. Could you add a comment pointing at the source so the two don't drift?
| retriever.release() | ||
| } | ||
| } | ||
| else -> null |
There was a problem hiding this comment.
(Shubham's Ai) aspectRatio is now undefined for non-media files on both platforms (previously 0 on Android, 1 on iOS). AttachmentPreview treats it as optional, so this looks fine, but please confirm a PDF share still previews correctly.
| NitroModules: {createHybridObject: jest.fn(() => ({processFiles: jest.fn()}))}, | ||
| })); | ||
|
|
||
| describe('ShareActionHandler native adapter', () => { |
There was a problem hiding this comment.
(Shubham's Ai) These tests stub processFiles and then assert that it returns the stubbed value, so they only exercise the mock. tests/ui/ShareRootPageTest.tsx covers the real behavior; I think this file can be dropped.
| let content = isText ? try String(contentsOf: url, encoding: .utf8) : url.absoluteString | ||
| var aspectRatio: Double? | ||
|
|
||
| if mimeType.hasPrefix("image/"), let image = UIImage(contentsOfFile: url.path), image.size.height > 0 { |
There was a problem hiding this comment.
(Shubham's Ai) Optional: UIImage(contentsOfFile:) decodes the whole image just to read its dimensions. CGImageSourceCopyPropertiesAtIndex reads only the header, which is cheaper for large photos. (The old ObjC code did the same, so not a regression.)
|
@nabi-ebrahimi Link https://github.com/Expensify/Mobile-Expensify/pull/14201 for HybridApp changes so we can make a build that works. |
Generate one shared-file contract for Swift, Kotlin, and TypeScript. Keep native storage internal and preserve the existing Onyx timestamp format.
Refs: #104432
Explanation of Change
This PR migrates the legacy
ShareActionHandlerReact Native bridge to a type-safe Nitro module within@expensify/nitro-utils.It replaces callback-based
NativeModulescommunication and platform-specific payload formats with a single generatedSharedFilecontract implemented in Kotlin for Android and Swift for iOS.processFiles()now returnsPromise<SharedFile[]>, eliminating Android JSON parsing and untyped native dictionaries in the Share flow, while safely falling back to an empty array on web.The native implementations retain existing share behavior for images, videos, PDFs, plain text, and
.txtfile attachments, while improving validation for missing, unreadable, empty, or malformed shared content. The Share screen now surfaces the existing attachment-error UI for empty results or native-processing rejections, preventing an endless loading skeleton.Fixed Issues
$ #104530
PROPOSAL:
Tests
/and verify it is handled as text, not as a missing file path.Offline tests
.zip, and verify the existing wrong-file-type error appears while offline.QA Steps
same as test
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)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.Screenshots/Videos
Android: Native
REC-20261022115641.mp4
Android: mWeb Chrome
N/A
iOS: Native
REC-20261022123326.mp4
iOS: mWeb Safari
N/A
MacOS: Chrome / Safari
N/A