Skip to content

feat: Add email contact picker when sending by email#620

Open
amariaux wants to merge 11 commits intomainfrom
contact-picker
Open

feat: Add email contact picker when sending by email#620
amariaux wants to merge 11 commits intomainfrom
contact-picker

Conversation

@amariaux
Copy link
Copy Markdown

@amariaux amariaux commented Apr 7, 2026

Ajout d'un bouton dans la zone de saisie pour l'envoi par email, pour ouvrir la sélection de contact native.

Note: Nouvelle méthode disponible pour android 17, à regarder à sa sortie.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 7, 2026

PR Reviewer Guide 🔍

(Review updated until commit a2b1da9)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Ineffective Exception Handling

The try-catch block in processContactPickerResultUri surrounds viewModelScope.launch, but exceptions thrown inside the coroutine (e.g., SecurityException, IllegalStateException from ContentResolver) will not be caught because launch returns immediately and executes asynchronously. Any exception in contactPickLaunch will propagate to the global exception handler and crash the app instead of being logged to Sentry. Move the try-catch block inside the coroutine.

fun processContactPickerResultUri(
    sessionUris: Uri,
    context: Context,
) {
    try {
        viewModelScope.launch { contactPickLaunch(sessionUris, context) }
    }catch (e: Exception){
        SentryLog.e(TAG, "Error while importing contacts from picker result", e)
    }
}
Empty Email Handling

In contactPickLaunch, the code does not filter out empty strings when reading email addresses from the contacts cursor (data1). Empty or blank strings from the contacts database will be added to the email list and displayed as invalid chips in the UI. Add a validation check to ensure data1?.isNotBlank() before processing it as an email address.

val data1 = cursor.getString(data1Idx) ?: ""

val email = if (mimeType == ContactsContract.CommonDataKinds.Email.CONTENT_ITEM_TYPE) data1 else null

val existingContact = contactsMap[lookupKey]
if (existingContact != null) {
    contactsMap[lookupKey] = existingContact.copy(
        emails = if (email != null) existingContact.emails + email else existingContact.emails,
    )
} else {
    contactsMap[lookupKey] = Contact(
        lookupKey = lookupKey,
        emails = if (email != null) listOf(email) else emptyList(),
    )

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 7, 2026

Persistent review updated to latest commit a604891

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 7, 2026

Persistent review updated to latest commit e08157e

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 7, 2026

Persistent review updated to latest commit 2bb029a

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 9, 2026

Persistent review updated to latest commit 573e64e

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 9, 2026

Persistent review updated to latest commit a83cc96

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 9, 2026

Persistent review updated to latest commit a2b1da9

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a native contact-picker affordance when sending a transfer by email, allowing users to select contacts and populate recipient email addresses.

Changes:

  • Adds a trailing icon slot to EmailAddressTextField and wires it through the decoration box.
  • Adds a trailing contact-picker button in PickFilesScreen to launch the native contacts email picker (multi-select when supported).
  • Adds ViewModel handling to read picked contact URIs via ContentResolver and append discovered emails to recipients.
  • Adds localized accessibility content-description strings and a new PersonsCircleAdd vector icon.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
app/src/main/res/values/strings.xml Adds default locale content description for the contact picker button.
app/src/main/res/values-sv/strings.xml Adds Swedish translation for the new content description.
app/src/main/res/values-pt/strings.xml Adds Portuguese translation for the new content description.
app/src/main/res/values-pl/strings.xml Adds Polish translation for the new content description.
app/src/main/res/values-nl/strings.xml Adds Dutch translation for the new content description.
app/src/main/res/values-nb/strings.xml Adds Norwegian Bokmål translation for the new content description.
app/src/main/res/values-it/strings.xml Adds Italian translation for the new content description.
app/src/main/res/values-fr/strings.xml Adds French translation for the new content description.
app/src/main/res/values-fi/strings.xml Adds Finnish translation for the new content description.
app/src/main/res/values-es/strings.xml Adds Spanish translation for the new content description.
app/src/main/res/values-el/strings.xml Adds Greek translation for the new content description.
app/src/main/res/values-de/strings.xml Adds German translation for the new content description.
app/src/main/res/values-da/strings.xml Adds Danish translation for the new content description.
app/src/main/java/com/infomaniak/swisstransfer/ui/screen/newtransfer/pickfiles/components/EmailAddressTextField.kt Exposes trailingIcon support to display the picker button inside the recipient field.
app/src/main/java/com/infomaniak/swisstransfer/ui/screen/newtransfer/pickfiles/PickFilesViewModel.kt Implements contact URI processing to extract and add recipient emails.
app/src/main/java/com/infomaniak/swisstransfer/ui/screen/newtransfer/pickfiles/PickFilesScreen.kt Adds the trailing icon button and launches the contact picker; forwards results to the ViewModel.
app/src/main/java/com/infomaniak/swisstransfer/ui/images/icons/PersonsCircleAdd.kt Introduces the contact-picker icon used by the new trailing button.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +417 to +420
try {
viewModelScope.launch { contactPickLaunch(sessionUris, context) }
} catch (e: Exception) {
SentryLog.e(TAG, "Error while importing contacts from picker result", e)
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The try/catch here won’t catch failures thrown inside the coroutine (it only wraps the synchronous call to viewModelScope.launch). If queryContacts/contentResolver throws (e.g., SecurityException), the exception will be uncaught in viewModelScope. Wrap the body of the launched coroutine in runCatching/try-catch (or install a CoroutineExceptionHandler) so errors are logged and don’t crash/cancel the scope.

Suggested change
try {
viewModelScope.launch { contactPickLaunch(sessionUris, context) }
} catch (e: Exception) {
SentryLog.e(TAG, "Error while importing contacts from picker result", e)
viewModelScope.launch {
try {
contactPickLaunch(sessionUris, context)
} catch (e: Exception) {
SentryLog.e(TAG, "Error while importing contacts from picker result", e)
}

Copilot uses AI. Check for mistakes.
Comment on lines +413 to +416
fun processContactPickerResultUri(
sessionUris: Uri,
context: Context,
) {
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ViewModel method requires a UI Context to be passed in, which forces the UI layer to thread a Context through callbacks. In this codebase, ViewModels that need a Context typically inject @ApplicationContext (e.g., SettingsSentryViewModel.kt:37-39). Consider injecting @ApplicationContext into PickFilesViewModel and using that ContentResolver instead, so callers only pass the picked Uri(s).

Copilot uses AI. Check for mistakes.
Comment on lines +464 to +466
private suspend fun updateValidatedRecipientsEmails(newEmails: Set<String>) = withContext(Dispatchers.Main) {
validatedRecipientsEmails = validatedRecipientsEmails + newEmails
}
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid hard-coding Dispatchers.Main inside a ViewModel when you already have injected dispatchers (ioDispatcher) and the caller coroutine resumes on Main after withContext(ioDispatcher). This extra context switch is unnecessary and makes testing harder; prefer updating state in the existing viewModelScope context or inject a Main dispatcher if needed.

Copilot uses AI. Check for mistakes.
selectedTransferType: GetSetCallbacks<TransferTypeUi>,
transferOptionsCallbacks: TransferOptionsCallbacks,
pickFiles: () -> Unit,
selectContact: (Uri, Context) -> Unit,
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The selectContact callback signature includes Context, which propagates Android framework types through the composable tree and makes the API harder to reuse/test. If the ViewModel injects @ApplicationContext (see comment in PickFilesViewModel), this callback can likely be simplified to only pass the picked Uri (or a list of Uris).

Suggested change
selectContact: (Uri, Context) -> Unit,
selectContact: (Uri) -> Unit,

Copilot uses AI. Check for mistakes.
@sonarqubecloud
Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants