Resubmit contacts import module #54459
Open
+4,499
−57
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…hub.com/margelo/expensify-app-fork; branch 'main' of https://github.com/Expensify/App into @perunt/rerevert-contacts-import-module
|
I didn't test the Hybrid App as I don't have access yet |
melvin-bot
bot
requested review from
DylanDylann and
shawnborton
and removed request for
a team
|
@shawnborton @DylanDylann One of you needs to 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] |
|
Hey! I've created a PR with adjustments so everything works fine with HybridApp |
|
Hi! @perunt what's your Xcode version locally? I'm testing PR on HybridApp and I cannot build for simulators (it builds fine on physical device tho). My version is I'm also curious about The key information here is that some people might encounter issues when building on a simulator, while others might not, depending on their Swift compiler version. This can block HybridApp development for people without physical iPhone |
|
Nevertheless I think this is not a serious issue and we can proceed further with this PR and its HybridApp part 😅 People experiencing issues with the Swift compiler can try downgrading their Xcode version or using a physical device if available. cc: @Julesssss |
|
I think only remaining thing is to bump Xcode on CI infrastructure. I have this PR that was created before Christmas: #54424 |
blimpich
previously approved these changes
Jan 7, 2025
|
Also I think we need to merge main due to xcode being bumped, which is why the validate github actions check is failing. |
…hub.com/margelo/expensify-app-fork; branch 'main' of https://github.com/Expensify/App into @perunt/rerevert-contacts-import-module
|
I'm still dealing with Android errors after bumping RN. I'll ping you once it's done |
…rerevert-contacts-import-module
…rerevert-contacts-import-module
|
I'm not sure about the lint error Apart from this, everything works on my end. As before, I didn't test the Hybrid App because I don't have access to it |
Ah this is from a new eslint rule we made. See announcement here. Its unrelated because we're trying to tackle the changes incrementally instead of doing it all at once in a big PR. So if you touch a file (ex: I'm fine with ignoring it for this PR, its big and complicated enough already that I don't want to add that to the list of things this PR changes, but for future reference we probably want to update those linter issues as we encounter them. |
blimpich
previously approved these changes
Jan 13, 2025
There was a problem hiding this comment.
Approving, checking with hybrid app engineers before merging: https://expensify.slack.com/archives/C04878MDF34/p1736803862458899?thread_ts=1736210982.318679&cid=C04878MDF34
|
Looks like C+ should have access to hybrid app now, @DylanDylann can we get your review on this? Obviously no need to review all the code again since you've reviewed the code throughly before, but I'd like you to test this before we merge since @perunt still doesn't have access to the hybrid app repo yet (working on fixing that). |
|
I verified that it works well on the Hybrid Android App (I am getting some problems while building a Hybrid IOS App, will try to give an update at the end of the day) Screen.Recording.2025-01-15.at.17.23.26.movScreen.Recording.2025-01-15.at.17.30.14.mov |
|
@perunt Could you please resolve conflict? |
@DylanDylann what kind of problems? |
|
@mateuuszzzzz No problem on Android, the IOS build failed because of this error 
|
Could you expand details of |
|
Yess 
|
|
Sorry, I meant this block 😅 |
|
🚧 @Julesssss has triggered a test hybrid app build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
First errorSecond Error (That you are asking) |
|
I think this may be due to our workflow nmv version. I think we are good on staging but not prod yet (where I think the workflow runs). |
…rerevert-contacts-import-module
|
@DylanDylann, the code is up to date |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation of Change
Resubmit contacts import module. Additionally was bumped Nitro
During the revert of the NitroModules PR, we identified two main issues affecting our iOS builds. First, there was a mismatch between the minimum iOS deployment target (set to 13.4) and our app's requirement (iOS 15). Second, we discovered build failures specific to iOS simulators due to a known Swift compiler issue.
While investigating, we found that our CI environment was running an older version of Xcode (15.2), which contributed to the build failures. We've addressed this by updating our CI configuration to use newer macOS runners and updating the Ruby GitHub Action version.
To move forward cleanly, we'll be splitting this into two separate PRs:
After implementing these changes in isolation, our test builds are now passing successfully.
revert #54421
Fixed Issues
$ #47938
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.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
MacOS: Desktop