Align NFC contact import and share dialogs with Design Standards v1.4#6332
Open
thebentern wants to merge 2 commits into
Open
Align NFC contact import and share dialogs with Design Standards v1.4#6332thebentern wants to merge 2 commits into
thebentern wants to merge 2 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThe changes preserve encoded NFC contact verification state, add NFC dialog colors and explanatory text, refactor shared-contact alert composition, and add preview and screenshot coverage. ChangesNFC alignment updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Open
4 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/share/SharedContactDialog.kt (2)
97-97: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unnecessary parentheses.
The parentheses around
node.user.public_key.sizeare redundant and can be safely removed for cleaner syntax.♻️ Proposed refactor
- if ((node.user.public_key.size) > 0 && node.user.public_key != sharedContact.user?.public_key) { + if (node.user.public_key.size > 0 && node.user.public_key != sharedContact.user?.public_key) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/share/SharedContactDialog.kt` at line 97, Update the condition in SharedContactDialog to remove the redundant parentheses around node.user.public_key.size while preserving the existing comparison and logical behavior.
56-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid obtaining
koinViewModel()as a default parameter in reusable composables.As per coding guidelines, obtain
koinViewModel()insideentry<T>blocks so ViewModels follow backstack lifetime. Providing it as a default parameter here can lead to incorrect scoping if this dialog is instantiated outside of its intended navigation destination context. Consider removing the default parameter to require the caller (which should be anentry<T>) to explicitly pass the ViewModel instance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/share/SharedContactDialog.kt` at line 56, Remove the koinViewModel() default from the SharedContactDialog viewModel parameter, requiring callers to provide the instance explicitly. Ensure the intended entry<T> navigation block obtains the ViewModel and passes it to SharedContactDialog, preserving backstack-scoped lifetime.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/share/SharedContactDialog.kt`:
- Line 97: Update the condition in SharedContactDialog to remove the redundant
parentheses around node.user.public_key.size while preserving the existing
comparison and logical behavior.
- Line 56: Remove the koinViewModel() default from the SharedContactDialog
viewModel parameter, requiring callers to provide the instance explicitly.
Ensure the intended entry<T> navigation block obtains the ViewModel and passes
it to SharedContactDialog, preserving backstack-scoped lifetime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5850b20e-9573-4307-822b-6cd619576b15
⛔ Files ignored due to path filters (6)
screenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/core/AlertScreenshotTestsKt/ScreenshotNfcWriteFailedAlert_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/core/AlertScreenshotTestsKt/ScreenshotNfcWriteFailedAlert_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/core/AlertScreenshotTestsKt/ScreenshotNfcWriteSuccessAlert_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/core/AlertScreenshotTestsKt/ScreenshotNfcWriteSuccessAlert_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/core/AlertScreenshotTestsKt/ScreenshotSharedContactImportAlert_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/core/AlertScreenshotTestsKt/ScreenshotSharedContactImportAlert_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.png
📒 Files selected for processing (9)
.skills/compose-ui/strings-index.txtcore/resources/src/commonMain/composeResources/values/strings.xmlcore/service/src/commonMain/kotlin/org/meshtastic/core/service/MessagingControllerImpl.ktcore/service/src/commonTest/kotlin/org/meshtastic/core/service/RadioControllerImplTest.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/AlertDialogs.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/QrDialog.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/share/SharedContactDialog.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/util/AlertPreviews.ktscreenshot-tests/src/screenshotTest/kotlin/org/meshtastic/screenshots/core/AlertScreenshotTests.kt
jamesarich
enabled auto-merge
July 21, 2026 00:36
github-merge-queue
Bot
removed this pull request from the merge queue due to no response for status checks
Jul 21, 2026
Implements the Android side of the cross-platform NFC alignment audit (meshtastic/design#126): - importContact now honors the manually_verified flag encoded by the sharer instead of forcing it to true, matching iOS (cross-platform policy from the design-repo NFC alignment audit). - The NFC write result dialog colors its message with SemanticColors.Success (Green 600) on success and colorScheme.error on failure (§7); MeshtasticDialog gained an optional messageColor. - New write_nfc_subtext explains what writing a tag does before the write starts (§6). - The shared-contact import dialog shows the NodeChip identity badge for both known and unknown contacts (§1). - Tests: importContact passthrough (false->false, true->true) with the sent AdminMessage payload asserted; screenshot goldens for the colored result dialogs and the import dialog (light + dark). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
core/resources/.../composeResources is Compose Multiplatform Resources, not an Android res/values dir, so AAPT's apostrophe-escaping rules do not apply and Compose Resources does not unescape \'. The escape was rendering literally as "Couldn\'t write to tag" in the dialog. Use a plain apostrophe, matching the ~25 strings already doing so in the same file (e.g. cant_change_no_radio, "Couldn't change channel"). Screenshot goldens for the failure dialog re-recorded. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thebentern
force-pushed
the
feat/nfc-tag-alignment
branch
2 times, most recently
from
July 21, 2026 11:28
4bc1658 to
896d582
Compare
thebentern
enabled auto-merge
July 21, 2026 11:28
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Implements the Android side of the cross-platform NFC tag alignment (audit: design#125, parent: design#126). Android is already the NFC reference implementation — these are Design-Standards-v1.4 conformance fixes plus one cross-platform semantic alignment.
What changes
manually_verifiedbit on import —MessagingControllerImpl.importContactno longer forcesmanually_verified = true; the flag encoded by the sharer is passed through to both theAdminMessage(add_contact)and the local NodeDB update. (OutboundsendSharedContactalready propagated the local flag; unchanged.) This matches iOS, whereaddContactFromURLhonors the encoded bit.SemanticColors.Success(Green 600#3FB86D), failure =MaterialTheme.colorScheme.error.MeshtasticDialoggained an optional message-color parameter (default keeps all existing call sites unchanged).write_nfc_subtextexplains what writing a tag does before the write starts (the existingwrite_nfc_textonly appears mid-write).NodeChip) instead of text only.importContactpassthrough (false→false, true→true) with the sentAdminMessagepayload asserted; screenshot previews for the colored success/failure dialogs and the import confirm.Design standards
Conforms to Meshtastic Client Design Standards v1.4 §1 (node identity), §6 (plain language), §7 (semantic colors — Success is Green 600, never the accent green).
Verification
spotlessCheck+detektclean;kmpSmokeCompilepasses:core:service:allTests+:core:ui:allTestspass — including the newimportContactpassthrough tests (false→false and true→true), which capture and assert the sentAdminMessage.add_contact.manually_verified:screenshot-tests:validateDebugScreenshotTestpasses (light + dark for the success, failure, and import dialogs)Note: the
.skills/compose-ui/strings-index.txtdiff includes catch-up entries for the pre-existingfirmware_update_*strings — the index onmainwas stale (#6309 didn't regenerate it), and running the requiredscripts/sort-strings.pyswept them in.Tracks meshtastic/design#126.
🤖 Generated with Claude Code
Closes #6331.
Summary by CodeRabbit