Repository navigation
Migrate ShareActionHandler to a type-safe Nitro module - #104435
roryabraham wants to merge 1 commit into
Conversation
Generate one shared-file contract for Swift, Kotlin, and TypeScript. Keep native storage internal and preserve the existing Onyx timestamp format. Refs: #104432
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@codex review |
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. |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
Explanation of Change
Replace the handwritten
NativeModules.ShareActionHandlercontract with a Nitrogen-generatedShareActionHandlerin the existing@expensify/nitro-utilsmodule. Swift and Kotlin now construct the same generatedSharedFilerecord and returnPromise<SharedFile[]>, rather than Android returning a JSON string and iOS returning dictionaries through a callback.processedAtas a string of epoch milliseconds to match the existing Onyx model; makeaspectRatioexplicitly optional./is not mistaken for a file path and a missing text file is not shared as literal text. Retain compatibility with the older intent writers.This branches from
mainindependently of #101208: it does not include that PR's cold-start persistence/routing changes. Reconcile the overlapping ShareRootPage and intent-handler changes before merging both.MOBILE-EXPENSIFY: https://github.com/Expensify/Mobile-Expensify/pull/14192
Merge the App implementation before removing the legacy hybrid bridges in the companion PR.
Manual test plan — not yet run
npm run specsinmodules/ExpensifyNitroUtils, and reinstall iOS pods/sync Gradle./. Verify the exact text is available in the Share flow, not interpreted as a missing attachment path..txtfile and verify it remains an attachment rather than being interpreted as literal text.new-expensify://share/root. Verify the existing attachment-error alert appears rather than leaving the screen on its skeleton.text/plainfile share withisText: false. Verify processing reports an attachment error and never shares the missing path as literal text.content/mimeType. Verify processing rejects and the Share screen displays an attachment error rather than crashing JS.main, treating the known Android cold-start routing issue in Fix cold-start file sharing to open Submit expense #101208 separately rather than expecting this migration to fix it.AI Tests
npm test -- --silent --maxWorkers=2was attempted and stopped at the five-minute limit before completion. It reported failures intests/unit/pages/HomePage.test.tsxbecauseuseIsFocused()could not find a navigation object. These are outside the changed Share code; they have not been baseline-verified or investigated here. No full-suite pass is claimed.git diff --checkpasses with generated end-of-line whitespace ignored.Fixed Issues
$ #104432
PROPOSAL: N/A — internal architectural follow-up to #101208.
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review".
// The numbered manual test plan above is proposed coverage, not a claim that device tests were performed.
Offline tests
Proposed; not run:
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
// Use the numbered manual and offline test plans above as the proposed QA coverage.
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari