Migrate TagReaderActivity to use BottomSheet and add confirmation buttons#6814
Migrate TagReaderActivity to use BottomSheet and add confirmation buttons#6814
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the NFC/QR tag handling flow to use a translucent overlay activity that presents a Compose modal bottom sheet requiring explicit user confirmation (“Allow once” / “Allow always”), persists an allow-list of approved tags, and adds developer tooling to clear that allow-list. It also extends tag URL parsing to support next.home-assistant.io in debug builds and adds unit/UI/screenshot coverage for the new flow.
Changes:
- Introduce a new tag approval/scanning UI (Compose bottom sheet) driven by a dedicated
TagReaderViewModel+TagReaderUiState - Persist and manage “approved tags” in
PrefsRepository(including a developer setting to clear them) - Extend NFC tag URL matching in debug builds and add new tests (unit + Compose UI + screenshot)
Reviewed changes
Copilot reviewed 23 out of 35 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| common/src/test/kotlin/io/homeassistant/companion/android/common/data/prefs/PrefsRepositoryImplTest.kt | Adds unit tests for the new approved-tags preference behavior |
| common/src/main/res/values/strings.xml | Adds strings for tag approval UI and clearing approved tags |
| common/src/main/kotlin/io/homeassistant/companion/android/util/UrlUtil.kt | Adds debug-only support for next.home-assistant.io tag URLs |
| common/src/main/kotlin/io/homeassistant/companion/android/common/data/prefs/PrefsRepositoryImpl.kt | Implements storing/listing/clearing approved tags |
| common/src/main/kotlin/io/homeassistant/companion/android/common/data/prefs/PrefsRepository.kt | Extends prefs API with approved-tags operations |
| common/src/main/kotlin/io/homeassistant/companion/android/common/compose/util/PainterResourceUtil.kt | Adds a Compose painter helper that can render adaptive icons |
| common/src/main/kotlin/io/homeassistant/companion/android/common/compose/composable/HAModalBottomSheet.kt | Adds configurable drag handle support to the themed bottom sheet wrapper |
| automotive/src/main/AndroidManifest.xml | Updates TagReader/Assist activity theming for translucent overlay |
| app/src/test/kotlin/io/homeassistant/companion/android/nfc/views/TagReaderScreenTest.kt | Adds Compose UI tests for tag approval/scanning/error/done behaviors |
| app/src/test/kotlin/io/homeassistant/companion/android/nfc/TagReaderViewModelTest.kt | Adds unit tests for ViewModel state transitions and scanning logic |
| app/src/screenshotTest/kotlin/io/homeassistant/companion/android/nfc/views/TagReaderScreenScreenshotTest.kt | Adds screenshot coverage for approving/scanning states |
| app/src/main/res/xml/preferences_developer.xml | Adds a developer setting entry to clear approved tags |
| app/src/main/res/values/styles.xml | Introduces a translucent overlay theme used by tag reader/assist activities |
| app/src/main/kotlin/io/homeassistant/companion/android/settings/developer/DeveloperSettingsPresenterImpl.kt | Implements clearing approved tags via prefs |
| app/src/main/kotlin/io/homeassistant/companion/android/settings/developer/DeveloperSettingsPresenter.kt | Exposes a presenter API to clear approved tags |
| app/src/main/kotlin/io/homeassistant/companion/android/settings/developer/DeveloperSettingsFragment.kt | Wires the new preference to clear approved tags and show a toast |
| app/src/main/kotlin/io/homeassistant/companion/android/nfc/views/TagReaderView.kt | Removes the prior full-screen “processing” UI |
| app/src/main/kotlin/io/homeassistant/companion/android/nfc/views/TagReaderScreen.kt | Adds the new bottom-sheet based tag approval/scanning UI |
| app/src/main/kotlin/io/homeassistant/companion/android/nfc/TagReaderViewModel.kt | Adds the new ViewModel driving approval + scanning + minimum scanning duration |
| app/src/main/kotlin/io/homeassistant/companion/android/nfc/TagReaderUiState.kt | Adds the sealed UI state model for the new flow |
| app/src/main/kotlin/io/homeassistant/companion/android/nfc/TagReaderActivity.kt | Refactors the activity to Compose + ViewModel-driven flow |
| app/src/main/AndroidManifest.xml | Updates TagReaderActivity to a translucent overlay + dedicated task settings |
| app/src/debug/AndroidManifest.xml | Adds debug-only intent filter for next.home-assistant.io/tag/… |
| override fun onCreate(savedInstanceState: Bundle?) { | ||
| super.onCreate(savedInstanceState) | ||
|
|
||
| val isNfcTag = intent.action == NfcAdapter.ACTION_NDEF_DISCOVERED | ||
| val isQrTag = intent.action == Intent.ACTION_VIEW | ||
|
|
||
| setContent { | ||
| HomeAssistantAppTheme { | ||
| TagReaderView() | ||
| HATheme { | ||
| val state by viewModel.uiState.collectAsStateWithLifecycle() | ||
|
|
||
| TagReaderScreen( | ||
| state = state, | ||
| onAllowOnce = viewModel::onAllowOnce, | ||
| onAllowAlways = viewModel::onAllowAlways, | ||
| onDismissed = viewModel::onDismissed, | ||
| onErrorAcknowledged = viewModel::onErrorAcknowledged, | ||
| onFinished = ::finish, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| lifecycleScope.launch { | ||
| if (intent.action == NfcAdapter.ACTION_NDEF_DISCOVERED || intent.action == Intent.ACTION_VIEW) { | ||
| val isNfcTag = intent.action == NfcAdapter.ACTION_NDEF_DISCOVERED | ||
|
|
||
| val url = | ||
| if (isNfcTag) { | ||
| NFCUtil.extractUrlFromNFCIntent(intent) | ||
| } else { | ||
| intent.data | ||
| } | ||
| try { | ||
| handleTag(url, isNfcTag) | ||
| } catch (e: Exception) { | ||
| showProcessingError(isNfcTag) | ||
| Timber.e(e, "Unable to handle url (${if (isNfcTag) "nfc" else "qr"}}): $url") | ||
| } | ||
| } | ||
| finish() | ||
| } | ||
| } | ||
|
|
||
| private suspend fun handleTag(url: Uri?, isNfcTag: Boolean) { | ||
| // https://www.home-assistant.io/tag/5f0ba733-172f-430d-a7f8-e4ad940c88d7 | ||
|
|
||
| val nfcTagId = UrlUtil.splitNfcTagId(url) | ||
| Timber.d("Tag ID: $nfcTagId") | ||
| if (nfcTagId != null && serverManager.isRegistered()) { | ||
| serverManager.servers().map { | ||
| lifecycleScope.async { | ||
| try { | ||
| serverManager.integrationRepository(it.id).scanTag(hashMapOf("tag_id" to nfcTagId)) | ||
| Timber.d("Tag scanned to HA successfully") | ||
| } catch (e: Exception) { | ||
| Timber.e(e, "Tag not scanned to HA") | ||
| } | ||
| } | ||
| }.awaitAll() | ||
| if (isNfcTag || isQrTag) { | ||
| val url = if (isNfcTag) NFCUtil.extractUrlFromNFCIntent(intent) else intent.data | ||
| viewModel.onIntentReceived(url = url, isNfcTag = isNfcTag) | ||
| } else { | ||
| showProcessingError(isNfcTag) | ||
| finish() | ||
| } |
There was a problem hiding this comment.
I'm not sure we need that, I didn't have any issue during my tests testing multiples.
| fun adaptiveIconPainterResource(@DrawableRes id: Int): Painter { | ||
| val res = LocalResources.current | ||
| val theme = LocalContext.current.theme | ||
|
|
||
| return if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { | ||
| // Android O supports adaptive icons, try loading this first (even though this is least likely to be the format). | ||
| val adaptiveIcon = ResourcesCompat.getDrawable(res, id, theme) as? AdaptiveIconDrawable | ||
| if (adaptiveIcon != null) { | ||
| BitmapPainter(adaptiveIcon.toBitmap().asImageBitmap()) | ||
| } else { | ||
| // We couldn't load the drawable as an Adaptive Icon, just use painterResource | ||
| painterResource(id) | ||
| } |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
I feel like this was intentional, at least when scanning NFC tags where you already have feedback that something was scanned. Maybe out of scope, but did you consider changing it to show a checkmark confirmation for a short while (like 1s) after the network request completed instead? Then the dialog offers real feedback. |
| android:name="io.homeassistant.companion.android.HiltComponentActivity" | ||
| /> | ||
|
|
||
| <activity |
There was a problem hiding this comment.
| <activity | |
| <!-- | |
| Support for next.home-assistant.io links when developing (merged with non-next links in main manifest). | |
| --> | |
| <activity |
| Column(modifier = Modifier.align(Alignment.TopCenter)) { | ||
| Image( | ||
| painter = adaptiveIconPainterResource(R.mipmap.ic_launcher_round), | ||
| contentDescription = stringResource(commonR.string.home_assistant_branding_icon_content_description), |
There was a problem hiding this comment.
As the app name is repeated below isn't this just for decoration and can we remove the description?
| android:launchMode="singleTask" | ||
| android:taskAffinity="io.homeassistant.companion.android.tag.reader" | ||
| android:autoRemoveFromRecents="true" | ||
| android:showWhenLocked="true" |
There was a problem hiding this comment.
This might be nice for some use cases but also removes the security of having to unlock the device to actually scan a tag. Now anyone with your device can trigger tag automations.
| xmlns:android="http://schemas.android.com/apk/res/android"> | ||
|
|
||
| <application tools:ignore="MissingApplicationIcon"> | ||
| <activity |
There was a problem hiding this comment.
| <activity | |
| <!-- | |
| Support for next.home-assistant.io links when developing (merged with non-next links in main manifest). | |
| --> | |
| <activity |
| android:launchMode="singleTask" | ||
| android:taskAffinity="io.homeassistant.companion.android.tag.reader" | ||
| android:autoRemoveFromRecents="true" | ||
| android:showWhenLocked="true" |
There was a problem hiding this comment.
See https://github.com/home-assistant/android/pull/6814/changes#r3203981076 + are cars ever locked?
| <string name="tag_approval_title">Allow sending?</string> | ||
| <string name="tag_approval_description">Sending the tag %1$s to perform automations attach to it.</string> | ||
| <string name="tag_allow_once">Allow once</string> | ||
| <string name="tag_allow_always">Allow always</string> |
There was a problem hiding this comment.
Are the strings already decided?
- "Allow sending" sounds weird to me, from the user perspective you're scanning a tag and the fact that scanning happens outside the app and the app only sends data is an implementation detail. (+ description should be adjusted if the title is adjusted)
- Maybe surround the tag ID with quotes to make it more obvious as user data like:
the tag 'abcdefgh' to perform - "to perform automations attach to it" .should be attached, but to better match the HA terminology I'd use something like "to trigger automations linked to it".
| ) { | ||
| Icon( | ||
| imageVector = Icons.Default.Close, | ||
| contentDescription = stringResource(commonR.string.cancel), |
There was a problem hiding this comment.
This should be set on the outer Box clickable modifier
| ), | ||
| contentAlignment = Alignment.Center, | ||
| ) { | ||
| Box( |
There was a problem hiding this comment.
Why is there a nested Box structure here? Can't you make the inner one clickable? Also for the touch ripple to properly be a circle.
| android:summary="@string/thread_debug_summary"/> | ||
| <Preference | ||
| android:key="tag_clear_allowed" | ||
| android:icon="@drawable/ic_delete" |
There was a problem hiding this comment.
I think mdi:tag-remove-outline would be a good candidate here.
| HAFilledButton( | ||
| text = stringResource(commonR.string.tag_allow_always), | ||
| onClick = onAllowAlways, | ||
| modifier = Modifier.fillMaxWidth(), | ||
| ) | ||
| HAPlainButton( | ||
| text = stringResource(commonR.string.tag_allow_once), | ||
| onClick = onAllowOnce, | ||
| modifier = Modifier.fillMaxWidth(), | ||
| ) |
There was a problem hiding this comment.
I don't think there are strong Material guidelines on once vs always buttons, so I'd align the order + which button is primary with iOS. Right now they're different orders and different primary buttons - I think iOS with once as primary + first option makes more sense, or both should receive equal importance.
Summary
This PR is replacing the full screen processing of a tag, with a bottom sheet with confirmation (Allow Once and Allow Always). It moves it to its own taskAfinity and made transparent so it can be shown on top of anything.
The design is inspired from the Android 17 biometrics (for the close button).
I've added in the settings a way to clear the allowed tags.
I've added support for
next.home-assistant.iolinks in debug which is useful while working with a local dev frontend.The previous implementation was blinking when scanning since it can go quite fast, I decided to show the bottom sheet for at least 1.5s to make it less blinky and still fast.
Checklist
Screenshots
Screen_recording_20260507_094005.mp4
Link to pull request in documentation repositories
User Documentation: home-assistant/companion.home-assistant#
Any other notes