Add contacts import nitro module #54459
Conversation
…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 |
|
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
…ter this file has been touched
|
@roryabraham thanks for your review! I’ve made some adjustments and moved the module to the Expensify namespace. I’ve also structured the code based on your comments. However, I left a few comments since I wasn't sure what exactly you meant |
|
@DylanDylann can you give this another look? |
|
🚧 @roryabraham has triggered a test app build. You can view the workflow run here. |
|
🚧 @roryabraham has triggered a test 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! 🧪🧪
|
|
|
||
| class HybridContactsModule : HybridContactsModuleSpec() { | ||
| @Volatile | ||
| private var estimatedMemorySize: Long = 0 |
There was a problem hiding this comment.
what would happen if we just remove this memory management code? Both Java and JavaScript are garbage-collected languages, so I'm wondering if this is truly required or if it could be a pre-optimization
| "specs-debug": "npm run --filter=\"**\" typescript && npx nitro-codegen --logLevel=\"debug\"", | ||
| "specs": "npx nitro-codegen" | ||
| }, | ||
| "author": "Marc Rousavy <[email protected]> (https://github.com/mrousavy)", |
There was a problem hiding this comment.
| "author": "Marc Rousavy <[email protected]> (https://github.com/mrousavy)", |
There was a problem hiding this comment.
The author field is a required option here. I can put Expensify there
| @@ -74,20 +87,25 @@ function MoneyRequestParticipantsSelector({ | |||
| }: MoneyRequestParticipantsSelectorProps) { | |||
| const {translate} = useLocalize(); | |||
| const styles = useThemeStyles(); | |||
| const [softPermissionModalVisible, setSoftPermissionModalVisible] = useState(false); | |||
There was a problem hiding this comment.
Remember that booleans should start with is, has, should, etc...
| const [softPermissionModalVisible, setSoftPermissionModalVisible] = useState(false); | |
| const [isSoftPermissionModalVisible, setSoftPermissionModalVisible] = useState(false); |
There was a problem hiding this comment.
Also, what do you mean by "soft permission"? What makes it "soft"? Would it be clearer to just call it isContactPermissionModalVisible?
There was a problem hiding this comment.
"Soft permission" refers to an app-controlled dialog that appears before triggering the actual system permission request.
It's called "soft" because it gives users context about why contact access is needed before showing the system's "hard" permission dialog. We use it for location and some other stuff
| import {getContactPermission} from '@libs/ContactPermission'; | ||
| import type {ContactPermissionModalProps} from './types'; | ||
|
|
||
| function ContactPermissionModal({startPermissionFlow, resetPermissionFlow, onDeny, onGrant}: ContactPermissionModalProps) { |
There was a problem hiding this comment.
I think we can substantially simplify this implementation:
diff --git a/src/components/ContactPermissionModal/index.native.tsx b/src/components/ContactPermissionModal/index.native.tsx
index 825c8bc4afb..2c5343974b0 100644
--- a/src/components/ContactPermissionModal/index.native.tsx
+++ b/src/components/ContactPermissionModal/index.native.tsx
@@ -1,4 +1,4 @@
-import React, {useEffect, useState} from 'react';
+import React, {useCallback, useEffect, useState} from 'react';
import {InteractionManager} from 'react-native';
import {RESULTS} from 'react-native-permissions';
import ConfirmModal from '@components/ConfirmModal';
@@ -8,49 +8,41 @@ import useThemeStyles from '@hooks/useThemeStyles';
import {getContactPermission} from '@libs/ContactPermission';
import type {ContactPermissionModalProps} from './types';
-function ContactPermissionModal({startPermissionFlow, resetPermissionFlow, onDeny, onGrant}: ContactPermissionModalProps) {
+function ContactPermissionModal({onGrant}: ContactPermissionModalProps) {
const [isModalVisible, setIsModalVisible] = useState(false);
const styles = useThemeStyles();
const {translate} = useLocalize();
useEffect(() => {
- if (!startPermissionFlow) {
- return;
- }
getContactPermission().then((status) => {
if (status === RESULTS.GRANTED || status === RESULTS.LIMITED) {
- return onGrant();
+ onGrant();
+ return;
}
if (status === RESULTS.BLOCKED) {
return;
}
setIsModalVisible(true);
});
- // eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps -- We only want to run this effect when startPermissionFlow changes
- }, [startPermissionFlow]);
+ // eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps -- We only want to run this effect on mount
+ }, []);
- const handleGrantPermission = () => {
+ const grantPermission = useCallback(() => {
setIsModalVisible(false);
InteractionManager.runAfterInteractions(onGrant);
- };
-
- const handleDenyPermission = () => {
- onDeny(RESULTS.DENIED);
- setIsModalVisible(false);
- };
+ }, [onGrant]);
- const handleCloseModal = () => {
+ const close = useCallback(() => {
setIsModalVisible(false);
- resetPermissionFlow();
- };
+ }, []);
return (
<ConfirmModal
isVisible={isModalVisible}
- onConfirm={handleGrantPermission}
- onCancel={handleDenyPermission}
- onBackdropPress={handleCloseModal}
+ onConfirm={grantPermission}
+ onCancel={close}
+ onBackdropPress={close}
confirmText={translate('common.continue')}
cancelText={translate('common.notNow')}
prompt={translate('contact.importContactsText')}
diff --git a/src/components/ContactPermissionModal/types.ts b/src/components/ContactPermissionModal/types.ts
index 5c831410656..d8cef3876d1 100644
--- a/src/components/ContactPermissionModal/types.ts
+++ b/src/components/ContactPermissionModal/types.ts
@@ -1,17 +1,6 @@
-import type {PermissionStatus} from 'react-native-permissions';
-
type ContactPermissionModalProps = {
/** A callback to call when the permission has been granted */
onGrant: () => void;
-
- /** A callback to call when the permission has been denied */
- onDeny: (permission: PermissionStatus) => void;
-
- /** Should start the permission flow? */
- startPermissionFlow: boolean;
-
- /** Reset the permission flow */
- resetPermissionFlow: () => void;
};
export default {};
diff --git a/src/pages/iou/request/MoneyRequestParticipantsSelector.tsx b/src/pages/iou/request/MoneyRequestParticipantsSelector.tsx
index ecbac67678a..c70abd23865 100644
--- a/src/pages/iou/request/MoneyRequestParticipantsSelector.tsx
+++ b/src/pages/iou/request/MoneyRequestParticipantsSelector.tsx
@@ -87,7 +87,6 @@ function MoneyRequestParticipantsSelector({
}: MoneyRequestParticipantsSelectorProps) {
const {translate} = useLocalize();
const styles = useThemeStyles();
- const [softPermissionModalVisible, setSoftPermissionModalVisible] = useState(false);
const [contactPermissionState, setContactPermissionState] = useState<PermissionStatus>(RESULTS.UNAVAILABLE);
const showImportContacts = !(contactPermissionState === RESULTS.GRANTED || contactPermissionState === RESULTS.LIMITED);
const [searchTerm, debouncedSearchTerm, setSearchTerm] = useDebouncedState('');
@@ -105,7 +104,7 @@ function MoneyRequestParticipantsSelector({
shouldInitialize: didScreenTransitionEnd,
});
const [contacts, setContacts] = useState<Array<SearchOption<PersonalDetails>>>([]);
- const [textInputAutoFocus, setTextInputAutoFocus] = useState(softPermissionModalVisible);
+ const [textInputAutoFocus, setTextInputAutoFocus] = useState(false);
const cleanSearchTerm = useMemo(() => debouncedSearchTerm.trim().toLowerCase(), [debouncedSearchTerm]);
const offlineMessage: string = isOffline ? `${translate('common.youAppearToBeOffline')} ${translate('search.resultsAreLimited')}` : '';
@@ -304,12 +303,8 @@ function MoneyRequestParticipantsSelector({
}, []);
useEffect(() => {
- setSoftPermissionModalVisible(true);
getContactPermission().then((status) => {
setContactPermissionState(status);
- if (status !== RESULTS.BLOCKED && status !== RESULTS.UNAVAILABLE) {
- setSoftPermissionModalVisible(true);
- }
});
}, []);
@@ -459,12 +454,7 @@ function MoneyRequestParticipantsSelector({
) : null;
}, [showImportContacts, inputHelperText, translate, styles, goToSettings]);
- const handleSoftPermissionDeny = useCallback(() => {
- setSoftPermissionModalVisible(false);
- }, []);
-
const handleSoftPermissionGrant = useCallback(() => {
- setSoftPermissionModalVisible(false);
InteractionManager.runAfterInteractions(handleContactImport);
setTextInputAutoFocus(true);
}, [handleContactImport]);
@@ -563,14 +553,7 @@ function MoneyRequestParticipantsSelector({
return (
<>
- {softPermissionModalVisible && (
- <ContactPermissionModal
- startPermissionFlow={softPermissionModalVisible}
- resetPermissionFlow={handleSoftPermissionDeny}
- onGrant={handleSoftPermissionGrant}
- onDeny={handleSoftPermissionDeny}
- />
- )}
+ <ContactPermissionModal onGrant={handleSoftPermissionGrant} />
<SelectionList
onConfirm={handleConfirmSelection}
sections={areOptionsInitialized ? sections : CONST.EMPTY_ARRAY}
There was a problem hiding this comment.
This approach is generally the same as the one we use in the app, so we’ll probably need to update it everywhere later
Hmm, but wait a minute, it looks like you completely removed startPermissionFlow.
Are you planning to remove it entirely, or just move it higher up in the component where it's used?
Also, it doesn't handle changing permissions anymore.
Just curious - was this generated by some kind of AI helper checker?
There was a problem hiding this comment.
I mean, if I were you, I'd use some. You’ve got tons of PRs to review every day 😄
There was a problem hiding this comment.
I didn't use any AI to generate this diff - I created it manually (although I admit I didn't test).
it looks like you completely removed startPermissionFlow
yeah, that was the intention. My version simplifies things in a couple ways:
- Remove
startPermissionFlow. Instead,ContactPermissionModaljust checks for permissions on mount, and if we haven't asked before, it renders the soft permissions modal. - That removes the need to manage the modal state in the parent component
|
Also, it looks like those test builds failed, so we should look into that |
That's odd. I checked both platforms before marking this PR as ready for review. Let me try it again |
|
Just checked the Hybrid App on iOS and Android and it's working well. I assume it'll be fine if we run it again |
|
🚧 @roryabraham has triggered a test 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! 🧪🧪
|
Hmm, seems like the iOS build failed with this error. @perunt could you take a look and see why this might be? The GH says that the Android build also failed but looking at the workflow it looks like it actually built fine. |
| @@ -0,0 +1,27 @@ | |||
| import {ContactsNitroModule} from '@expensify/nitro-utils'; | |||
| import type {Contact} from '@expensify/nitro-utils'; | |||
| import {CONTACT_FIELDS} from '@expensify/nitro-utils/src/specs/ContactsModule.nitro'; | |||
There was a problem hiding this comment.
Could we move CONTACT_FIELDS defination to the modules/ExpensifyNitroUtils/src/index.ts file? Then we don't need to import from the deep file in the module
There was a problem hiding this comment.
If do that we also should remove "@expensify/nitro-utils/*": ["modules/ExpensifyNitroUtils/*"] line in tsconfig.json
There was a problem hiding this comment.
Or we still define CONTACT_FIELDS in ContactsModule.nitro but we export it to outside of module in the index.ts file
| .then((deviceContacts) => ({ | ||
| contactList: Array.isArray(deviceContacts) ? deviceContacts : [], | ||
| permissionStatus, | ||
| })); |
There was a problem hiding this comment.
It will be better to add a catch block here
.catch((error) => {
+ console.error('Error importing contacts:', error);
+ return {
+ contactList: [],
+ permissionStatus,
+ };
+ });
| s.license = package["license"] | ||
| s.authors = package["author"] | ||
|
|
||
| s.platforms = { :ios => min_ios_version_supported } |
There was a problem hiding this comment.
is min_ios_version_supported defined automatically somewhere?
| thumbnailImageData?: string; | ||
| }; | ||
|
|
||
| type ContactFields = 'FIRST_NAME' | 'LAST_NAME' | 'PHONE_NUMBERS' | 'EMAIL_ADDRESSES' | 'IMAGE_DATA'; |
There was a problem hiding this comment.
| type ContactFields = 'FIRST_NAME' | 'LAST_NAME' | 'PHONE_NUMBERS' | 'EMAIL_ADDRESSES' | 'IMAGE_DATA'; | |
| type ContactFields = keyof typeof CONTACT_FIELDS; |
| useEffect(() => { | ||
| setSoftPermissionModalVisible(true); | ||
| getContactPermission().then((status) => { | ||
| setContactPermissionState(status); | ||
| if (status !== RESULTS.BLOCKED && status !== RESULTS.UNAVAILABLE) { | ||
| setSoftPermissionModalVisible(true); | ||
| } | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
This useEffect is confused to me. we always set setSoftPermissionModalVisible is true initially
| return onGrant(); | ||
| } | ||
| if (status === RESULTS.BLOCKED) { | ||
| return; |
There was a problem hiding this comment.
Should we call onDeny here?
Explanation of Change
https://github.com/Expensify/Mobile-Expensify/pull/13516 - use this PR for Mobile-Expensify
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