feat(notifications): align message notifications with Android best practices

[jamesarich]

Why

A review of MeshNotificationManagerImpl against Android's messaging-notification guidance surfaced three gaps:

1. Every incoming message rebuilt the recent conversation from the database and re-posted already-read messages as new content. MessagingStyle is designed to accumulate, and it has a first-class mechanism for context that shouldn't re-alert — we weren't using it.
2. All notification types shared one integer ID space. IDs came from contactKey.hashCode(), name.hashCode(), and raw node.num, alongside fixed SUMMARY_ID = 1 / SERVICE_NOTIFY_ID = 101. A node whose num == 101 would overwrite the foreground-service notification; num == 1 would collide with the group summary; an alert and a message could collide via unrelated hashes.
3. Conversation shortcuts were published but never linked. feature/car published long-lived conversation shortcuts (Person + LocusId + SHORTCUT_CATEGORY_CONVERSATION), but only during an Android Auto session, and no notification ever set setShortcutId/setLocusId — so DMs and channels never received the Conversations-section treatment (Android 11+), and the ensureConversationShortcut helper was dead code. The two halves were built and never connected.

What

🌟 Conversations-section support on phones

Conversation-shortcut publishing moves out of feature/car into a new ConversationShortcutPublisher in core:service, owned by the MeshService lifecycle (startObserving in onCreate, stopObserving in onDestroy) so shortcuts exist whenever the service runs — not only in Android Auto. Conversation notifications now ensureConversationShortcut(...) then set setShortcutId + setLocusId, so Android ranks them in the shade's Conversations section and surfaces them to Auto/Wear. The car-only ConversationShortcutManager is retired (Auto keeps working — the same shortcuts are now published for the whole service lifetime). The shortcut deep-link is aligned to the canonical $DEEP_LINK_BASE_URI/messages/... the notification tap already uses.

🛠️ Read messages become historic context, not new content

Already-read messages are added via MessagingStyle.addHistoricMessage() instead of addMessage(), so they provide conversation context to accessibility services and Auto/Wear without being re-presented as freshly-arrived. Unread messages — and reactions, which are themselves new events — remain the alerting content. A guard keeps the most-recent message alerting when everything in the window is already read (e.g. a reaction on a read message).

🐛 Per-type notification ID namespacing

Every type now posts through notify(tag, id, …) / cancel(tag, id), eliminating the cross-type ID collisions above. The group summary is excluded from the active-conversation count by its FLAG_GROUP_SUMMARY flag rather than a fragile id != SUMMARY_ID check.

🐛 Orphaned group summary after mark-as-read / reply

Found during on-emulator testing: after the last conversation was dismissed via mark-as-read or reply, an empty group summary lingered in the shade (and Android Auto). NotificationManager.activeNotifications doesn't reflect a cancel() issued moments earlier, so showGroupSummary() rebuilt the summary from stale state instead of dropping it — defeating the existing activeNotifications.isEmpty() guard. cancelMessageNotification now passes the just-cancelled id to showGroupSummary, which excludes it from the stale snapshot so the summary is correctly cleared.

Device-switch state reset — verified, no code change needed

switchDevice() clears notifications (cancelAll); conversation shortcuts reset through the publisher's existing flows: DM shortcuts via the currentDb-reactive contacts flow, and channel shortcuts via channelSetFlow + the publisher's stale-shortcut removal. Both were confirmed on-emulator (see Testing). An earlier draft added an identity-keyed wipe for this; testing showed it redundant (the existing stale-removal already resets channels when a device connects) so it was dropped.

🌟 Styled conversation shortcuts (channels vs DMs)

Previously every conversation shortcut had icon=null, so launchers / Android Auto / the People space rendered them all as identical generic head silhouettes with only a text label. Now each shortcut carries a rendered icon that distinguishes the two conversation types at a glance:

• DMs — a wide-pill, node-colored avatar showing the node's short name (mirroring the in-app node chip); shortLabel = short name, longLabel = long name.
• Channels — a black rounded-square badge with the channel number (uncolored, so channels read as a consistent group); label is the effective channel name, resolving an empty primary to its modem-preset name (e.g. LongFast) to match the in-app conversation list rather than a placeholder.

The icon is set on the shortcut itself (not just the Person), which is what fixes the generic-head rendering. PersonIconFactory gained a shape-aware, auto-sizing label renderer shared by both paths (and the on-demand ensureConversationShortcut, so there's no generic-head flash before the observer republishes).

In-notification sender avatars (the MessagingStyle Person icons) also now hold each participant's full short name — e.g. WOLF, HAWK, 2c3d — rather than just its first character, and use the same wide-pill shape, so senders are identifiable at a glance in a busy channel conversation.

Shortcuts are ranked by recency: DMs and channels are merged and sorted by last-activity time (from the message DB), and setRank is applied so the most recently active conversations occupy the low ranks. Launchers / Android Auto show only a small capped number of dynamic shortcuts, so without this the surfaced set was an arbitrary slice of stale channels; now it's the conversations the user actually touched most recently.

🛠️ Modern-API review fixes

A holistic pass against current notification guidance (Android 12–16 behaviors) landed five more refinements:

• Reply confirmation flow — an inline reply now re-posts the conversation silently with the sent reply appended (refreshConversationAfterReply, new interface method with a dismiss fallback default), instead of the notification vanishing. This is the MessagingStyle confirmation pattern and resolves the RemoteInput spinner with visible feedback; on Android Auto the conversation shows the reply rather than disappearing. A failed send still dismisses so the spinner never hangs.
• No double alert from the group summary — the summary now sets GROUP_ALERT_CHILDREN; previously it was posted on a HIGH-importance channel with default alert behavior, so it could buzz a second time for every message.
• FOREGROUND_SERVICE_IMMEDIATE — Android 12+ can defer FGS notifications ~10s; the connection-state notification now shows immediately (the "Connecting…" window is exactly when the user is watching).
• Person.setImportant for favorites — favorite nodes feed the system's conversation-priority ranking (shade ordering, suggestions), on both notification senders and DM shortcut persons.
• Brand accent color — notifications use the Meshtastic Green 500 accent (#67EA94, per design standards) instead of raw Color.BLUE.

Conversations surface — scope note

Message notifications now get the full Conversations treatment on Android 11+: long-lived SHORTCUT_CATEGORY_CONVERSATION shortcuts + setShortcutId/setLocusId, verified found valid? true for both DMs (my_messages) and broadcasts (my_broadcasts). Bubbles / chat-heads are intentionally not enabled here — they need an embeddable bubble Activity plus BubbleMetadata and per-channel setAllowBubbles, which is a separate feature. This PR lays the required shortcut/LocusId foundation that bubbles build on; enabling them can follow.

🧹 Avatar caching + dedupe

Generated avatars are cached per node instead of re-rasterizing a 128px bitmap for every message on every rebuild. PersonIconFactory moves to core:service and now backs both the shortcut publisher and the notification avatars, removing the duplicate that lived in feature/car.

Testing performed

• ./gradlew spotlessApply spotlessCheck detekt assembleDebug test allTests — full baseline, both flavors.
• :androidApp:testFdroidDebugUnitTest --tests "*KoinVerificationTest*" — verifies the new ConversationShortcutPublisher binding and MeshNotificationManagerImpl's added dependencies resolve across the DI graph.
• New regression tests (Robolectric, androidHostTest):
  • MeshNotificationManagerImplConversationTest — read messages become historic / unread stay alerting; per-type (tag, id) namespacing (a num == 101 node no longer clobbers the FGS notification); summary uses GROUP_ALERT_CHILDREN and is dropped with the last conversation; refreshConversationAfterReply re-posts with the effective channel name (broadcast) and without a conversation title (DM); brand accent color.
  • ConversationShortcutPublisherTest — recency ranking across DMs and channels (setRank), DM short/long labels, empty-primary → preset name ("LongFast"), stale-shortcut pruning.
  • ReplyReceiverTest updated: a successful reply refreshes the conversation in place (and never cancels); a failed send still dismisses.
• Updated MeshNotificationManagerImplTest for the new constructor parameters.

Live E2E on emulator (fdroid debug) driven by meshtastic-mcp replay

Connected the app (TCP) to a simulated 800-node mesh and injected messages, inspecting dumpsys notification:

• Shortcut linkage (broadcast + DM) — a broadcast reports shortcut=0^all found valid? true on my_broadcasts; a direct message reports shortcut=0!<hex> found valid? true with locusId on my_messages. Both DM and channel conversation shortcuts are published (SHORTCUT_CATEGORY_CONVERSATION) and accepted by the system.
• ID namespacing — message (id 47226035), message_summary (id 1), and the foreground service (id 101, untagged) coexist as distinct records; none clobbers another.
• Historic vs new — after marking two messages read then receiving one new message, the notification carries android.messages=Parcelable[] (1) (the new message) and android.messages.historic=Parcelable[] (2) (the read context), with android.text = the new message. Read history is context, not new content.
• Orphan summary fix — mark-as-read now clears both the message and the summary (only the foreground-service notification remains).
• Device-switch reset — switching the device address (new DB) logged Published 0 conversation shortcuts then repopulated (DM reset via the contacts flow); reconnecting to a different mesh scenario flipped the channel shortcuts from one channel set to the other (MeshCon/Talks/Swap/Hax/Staff → Everyone/BRC/Playa/Rangers) via channelSetFlow + stale-removal, with stale channels dropped. Notifications cleared via cancelAll.

Remaining (non-blocking): worth a real-device eyeball for the shade's Conversations-section presentation. Note the replay tooling presents a single fixed local node identity regardless of seed/scenario, so a true two-identity device switch (vs. the address/DB and channel-set switches exercised here) still wants a two-radio check.

Summary by CodeRabbit

• New Features
  • Added Android dynamic conversation shortcuts for direct messages and channels, ranked by recent activity, with deep links and improved shortcut labels.
  • Inline replies now refresh the conversation notification so sent messages appear immediately.
• Bug Fixes
  • Improved conversation notifications for read/unread context, correct group summary handling, and stable notification identifiers to prevent unrelated alerts from being affected.
• Tests
  • Added Robolectric coverage for shortcut ranking/labeling, stale shortcut removal, notification refresh after replies, and conversation-notification semantics.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 90469016-2b42-4ccd-859c-b3e9e1fcf6bd

📥 Commits

Reviewing files that changed from the base of the PR and between d7cff2d and f2a08ff.

📒 Files selected for processing (3)

• core/service/src/androidHostTest/kotlin/org/meshtastic/core/service/ConversationShortcutPublisherTest.kt
• core/service/src/androidMain/kotlin/org/meshtastic/core/service/ConversationShortcutPublisher.kt
• core/service/src/androidMain/kotlin/org/meshtastic/core/service/MeshNotificationManagerImpl.kt

🚧 Files skipped from review as they are similar to previous changes (3)

• core/service/src/androidHostTest/kotlin/org/meshtastic/core/service/ConversationShortcutPublisherTest.kt
• core/service/src/androidMain/kotlin/org/meshtastic/core/service/ConversationShortcutPublisher.kt
• core/service/src/androidMain/kotlin/org/meshtastic/core/service/MeshNotificationManagerImpl.kt

---

📝 Walkthrough

Walkthrough

Conversation shortcuts are managed by the core service, while notifications use conversation metadata, stable identifiers, cached icons, historic message context, and refreshed inline replies. Android Auto’s dedicated shortcut manager was removed.

Changes

Conversation notifications

Layer / File(s)	Summary
Shortcut publishing and lifecycle core/service/src/androidMain/..., core/service/src/androidHostTest/..., feature/car/src/main/...	Conversation shortcuts are ranked, labeled, rendered, pruned, tested, and observed for the service lifetime.
Conversation notification model core/service/src/androidMain/kotlin/org/meshtastic/core/service/MeshNotificationManagerImpl.kt, core/service/src/androidMain/kotlin/org/meshtastic/core/service/PersonIconFactory.kt	Notifications use stable tag/ID pairs, shortcut and locus metadata, cached person icons, historic read messages, effective conversation names, and updated group summaries.
Inline-reply refresh contract and flow core/repository/src/commonMain/..., core/service/src/androidMain/..., core/service/src/androidHostTest/...	Inline replies refresh the conversation after sending and marking it read; send or refresh failures cancel the notification.
Conversation notification validation core/service/src/androidHostTest/...	Robolectric tests cover shortcut ranking and pruning, message history, summaries, notification namespacing, channel titles, DM labels, and reply refresh.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

• meshtastic/Meshtastic-Android#6196: Both changes update inline-reply handling around ReplyReceiver and SendMessageUseCase.

Suggested labels: ui

Sequence Diagram(s)

sequenceDiagram
  participant ReplyReceiver
  participant SendMessageUseCase
  participant MeshNotificationManagerImpl
  participant ConversationShortcutPublisher
  participant ShortcutManager

  ReplyReceiver->>SendMessageUseCase: Send inline reply
  SendMessageUseCase-->>ReplyReceiver: Return send result
  ReplyReceiver->>MeshNotificationManagerImpl: Refresh conversation notification
  MeshNotificationManagerImpl->>ConversationShortcutPublisher: Ensure conversation shortcut
  ConversationShortcutPublisher->>ShortcutManager: Publish shortcut if absent
  MeshNotificationManagerImpl-->>ReplyReceiver: Repost conversation notification

Loading

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main notification-focused changes and matches the PR's overall intent.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[jamesarich]

@copilot review

[jamesarich]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

core/service/src/androidMain/kotlin/org/meshtastic/core/service/ConversationShortcutPublisher.kt (1)

70-83: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Shortcut publishing runs IPC + bitmap rendering on the Main dispatcher.

startObserving is invoked with MeshService.serviceScope, which uses Dispatchers.Main.immediate. Every collect emission calls publishShortcuts synchronously, which performs ShortcutManagerCompat IPC (getDynamicShortcuts, pushDynamicShortcut, removeDynamicShortcuts) and re-rasterizes bitmaps for every shortcut on the Main thread. Since the observed flow re-emits on every recency-affecting DB change, this repeats for the life of the foreground service and can add avoidable jank to the shared Main thread.

Move the collection to a background dispatcher.

♻️ Proposed fix

+import org.meshtastic.core.common.util.ioDispatcher
...
     fun startObserving(scope: CoroutineScope) {
         observeJob?.cancel()
         observeJob =
-            scope.launch {
+            scope.launch(ioDispatcher) {
                 combine(packetRepository.getContacts(), radioConfigRepository.channelSetFlow) { contacts, channelSet ->
                     rankConversationsByRecency(contacts, channelSet)
                 }
                     .distinctUntilChanged()
                     .collect { conversations -> publishShortcuts(conversations) }
             }
     }

🤖 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/service/src/androidMain/kotlin/org/meshtastic/core/service/ConversationShortcutPublisher.kt`
around lines 70 - 83, Update startObserving so the launched observation
coroutine collects the combined flow on a background dispatcher, rather than
inheriting MeshService.serviceScope’s Main dispatcher. Keep the existing
ranking, distinctUntilChanged, and publishShortcuts flow unchanged while
ensuring publishShortcuts and its shortcut IPC/bitmap work execute off Main.

core/service/src/androidMain/kotlin/org/meshtastic/core/service/MeshNotificationManagerImpl.kt (1)

98-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

ConcurrentHashMap should be replaced per project convention for androidMain.

personIconCache uses java.util.concurrent.ConcurrentHashMap. The repo's own guideline for androidMain (not just commonMain) explicitly calls for replacing this.

A Mutex-guarded mutableMapOf() (or atomicfu) would require making cachedPersonIcon suspend or wrapping access in synchronized/Mutex.withLock, since call sites here are non-suspend. Given the low contention (single service-owned cache), a Mutex-guarded plain map, or simply Collections.synchronizedMap-style guarding, would satisfy the convention without much churn.
As per coding guidelines, "Replace ConcurrentHashMap with atomicfu or a Mutex-guarded mutableMapOf(), java.util.concurrent.locks.* with Mutex, and java.io.* with Okio BufferedSource/BufferedSink."

Also applies to: 153-159

🤖 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/service/src/androidMain/kotlin/org/meshtastic/core/service/MeshNotificationManagerImpl.kt`
at line 98, Replace the ConcurrentHashMap-based personIconCache in
MeshNotificationManagerImpl with a Mutex-guarded mutableMapOf, preserving
thread-safe access in cachedPersonIcon and the related cache operations around
the second referenced block. Since these call sites are non-suspend, use
appropriate locking or adjust the access path so synchronization remains correct
without retaining java.util.concurrent.ConcurrentHashMap.

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.

Inline comments:
In
`@core/service/src/androidMain/kotlin/org/meshtastic/core/service/ConversationShortcutPublisher.kt`:
- Around line 226-259: Remove the raw contactKey interpolation from both
exception messages in ensureConversationShortcut. Keep the failure context while
logging only a generic on-demand shortcut publication message, consistent with
the aggregate logging in publishShortcuts.

In
`@core/service/src/androidMain/kotlin/org/meshtastic/core/service/ReplyReceiver.kt`:
- Around line 77-97: Update the coroutine error handling in ReplyReceiver’s
reply-send flow to use kotlinx.coroutines.CancellationException and safeCatching
for the refreshConversationAfterReply and business-logic cancellation paths,
ensuring cancellation is rethrown rather than logged or swallowed. Retain
runCatching only for the notification cancellation cleanup/teardown calls.

---

Nitpick comments:
In
`@core/service/src/androidMain/kotlin/org/meshtastic/core/service/ConversationShortcutPublisher.kt`:
- Around line 70-83: Update startObserving so the launched observation coroutine
collects the combined flow on a background dispatcher, rather than inheriting
MeshService.serviceScope’s Main dispatcher. Keep the existing ranking,
distinctUntilChanged, and publishShortcuts flow unchanged while ensuring
publishShortcuts and its shortcut IPC/bitmap work execute off Main.

In
`@core/service/src/androidMain/kotlin/org/meshtastic/core/service/MeshNotificationManagerImpl.kt`:
- Line 98: Replace the ConcurrentHashMap-based personIconCache in
MeshNotificationManagerImpl with a Mutex-guarded mutableMapOf, preserving
thread-safe access in cachedPersonIcon and the related cache operations around
the second referenced block. Since these call sites are non-suspend, use
appropriate locking or adjust the access path so synchronization remains correct
without retaining java.util.concurrent.ConcurrentHashMap.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 6062284f-c817-4e2a-9246-1849dfdc3c0f

📥 Commits

Reviewing files that changed from the base of the PR and between 99d5d1a and fdf099a.

📒 Files selected for processing (13)

• core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/MeshNotificationManager.kt
• core/service/src/androidHostTest/kotlin/org/meshtastic/core/service/ConversationShortcutPublisherTest.kt
• core/service/src/androidHostTest/kotlin/org/meshtastic/core/service/MeshNotificationManagerImplConversationTest.kt
• core/service/src/androidHostTest/kotlin/org/meshtastic/core/service/MeshNotificationManagerImplTest.kt
• core/service/src/androidHostTest/kotlin/org/meshtastic/core/service/ReplyReceiverTest.kt
• core/service/src/androidMain/kotlin/org/meshtastic/core/service/ConversationShortcutPublisher.kt
• core/service/src/androidMain/kotlin/org/meshtastic/core/service/MeshNotificationManagerImpl.kt
• core/service/src/androidMain/kotlin/org/meshtastic/core/service/MeshService.kt
• core/service/src/androidMain/kotlin/org/meshtastic/core/service/PersonIconFactory.kt
• core/service/src/androidMain/kotlin/org/meshtastic/core/service/ReplyReceiver.kt
• feature/car/src/main/kotlin/org/meshtastic/feature/car/service/ConversationShortcutManager.kt
• feature/car/src/main/kotlin/org/meshtastic/feature/car/service/MeshtasticCarSession.kt
• feature/car/src/main/kotlin/org/meshtastic/feature/car/util/PersonIconFactory.kt

💤 Files with no reviewable changes (3)

• feature/car/src/main/kotlin/org/meshtastic/feature/car/util/PersonIconFactory.kt
• feature/car/src/main/kotlin/org/meshtastic/feature/car/service/ConversationShortcutManager.kt
• feature/car/src/main/kotlin/org/meshtastic/feature/car/service/MeshtasticCarSession.kt

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

core/service/src/androidHostTest/kotlin/org/meshtastic/core/service/MeshNotificationManagerImplConversationTest.kt (1)

227-237: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert the DM shortcut label, not only the missing conversation title.

This test claims to validate DM peer-name resolution for the shortcut label, but only checks that a group conversation title is absent. A regression publishing an empty or incorrect DM shortcut label would still pass. Assert the published shortcut label is "Hawk Ridge" or rename the test to cover only title behavior.

🤖 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/service/src/androidHostTest/kotlin/org/meshtastic/core/service/MeshNotificationManagerImplConversationTest.kt`
around lines 227 - 237, Update the test `refreshConversationAfterReply resolves
a dm peer name for the shortcut label` to assert that the published notification
shortcut label equals "Hawk Ridge", in addition to or instead of the existing
conversation-title assertion. Ensure the assertion targets the notification
shortcut metadata produced by `refreshConversationAfterReply`.

🤖 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.

Outside diff comments:
In
`@core/service/src/androidHostTest/kotlin/org/meshtastic/core/service/MeshNotificationManagerImplConversationTest.kt`:
- Around line 227-237: Update the test `refreshConversationAfterReply resolves a
dm peer name for the shortcut label` to assert that the published notification
shortcut label equals "Hawk Ridge", in addition to or instead of the existing
conversation-title assertion. Ensure the assertion targets the notification
shortcut metadata produced by `refreshConversationAfterReply`.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 9c1ee78c-09db-4afd-878c-60cbe3f58cac

📥 Commits

Reviewing files that changed from the base of the PR and between fdf099a and 220ccc8.

📒 Files selected for processing (2)

• core/service/src/androidHostTest/kotlin/org/meshtastic/core/service/MeshNotificationManagerImplConversationTest.kt
• core/service/src/androidMain/kotlin/org/meshtastic/core/service/MeshNotificationManagerImpl.kt

🚧 Files skipped from review as they are similar to previous changes (1)

• core/service/src/androidMain/kotlin/org/meshtastic/core/service/MeshNotificationManagerImpl.kt

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

core/service/src/androidMain/kotlin/org/meshtastic/core/service/ConversationShortcutPublisher.kt (3)

156-160: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Do not expose contactKey as the fallback display label.

When node metadata is unavailable, the raw identifier is rendered as the shortcut’s short and long label. Use a localized generic fallback; keep contactKey only in internal IDs and deep-link metadata.

As per coding guidelines, “Never log or expose personally identifiable information, location data, or cryptographic keys.” As per path instructions, “Flag leftover // ... existing code ... placeholders, and any logging of PII, location, or cryptographic keys.”

🤖 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/service/src/androidMain/kotlin/org/meshtastic/core/service/ConversationShortcutPublisher.kt`
around lines 156 - 160, Update the label fallback logic in
ConversationShortcutPublisher so shortLabel and longLabel use the existing
localized generic fallback when node metadata is missing, rather than
dm.contactKey. Keep contactKey restricted to internal identifiers and deep-link
metadata, and flag any leftover placeholders or logging that exposes it.

Sources: Coding guidelines, Path instructions

---

193-193: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Localize the fallback channel label.

"Channel ${channel.index}" is user-facing shortcut text and should come from core:resources, using the project’s nonblocking resource-loading path.

As per coding guidelines, “Do not hardcode user-facing strings. Use core:resources…”

🤖 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/service/src/androidMain/kotlin/org/meshtastic/core/service/ConversationShortcutPublisher.kt`
at line 193, Replace the hardcoded fallback in the channelName assignment with
the localized channel-label resource from core:resources, loading it through the
project’s established nonblocking resource-access path and passing channel.index
as the format argument. Preserve channel.name when non-empty.

Source: Coding guidelines

---

137-140: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Do not prune on-demand shortcuts from an incomplete snapshot.

publishShortcuts can remove the shortcut created by ensureConversationShortcut before the contacts flow includes that conversation, breaking the immediate notification-linking contract. Scope cleanup to publisher-owned IDs and preserve pending on-demand shortcuts until the observed snapshot catches up.

Also applies to: 224-232

🤖 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/service/src/androidMain/kotlin/org/meshtastic/core/service/ConversationShortcutPublisher.kt`
around lines 137 - 140, Update publishShortcuts cleanup to avoid removing
on-demand shortcuts while the contacts snapshot is incomplete. Restrict stale-ID
pruning to shortcuts owned by the publisher, and preserve shortcuts created by
ensureConversationShortcut until the observed contacts flow includes their
conversation; apply the same ownership and preservation logic to the cleanup
path around the additional referenced block.

🧹 Nitpick comments (1)

core/service/src/androidMain/kotlin/org/meshtastic/core/service/ConversationShortcutPublisher.kt (1)

81-85: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Observe node metadata changes as well as message recency.

buildDmShortcut derives labels, icons, and favorite priority from nodeRepository.nodeDBbyNum.value, but this flow only combines contacts and channel configuration. Unless getContacts() emits for node renames/favorite changes, shortcuts remain stale. Add a node-database/version trigger and regression tests.

🤖 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/service/src/androidMain/kotlin/org/meshtastic/core/service/ConversationShortcutPublisher.kt`
around lines 81 - 85, Update the shortcut-publishing flow around
rankConversationsByRecency to also collect a node-database or version trigger
from nodeRepository, so buildDmShortcut-derived labels, icons, and favorite
priority refresh when node metadata changes. Preserve the existing contacts and
channel-set inputs, and add regression coverage for node rename and
favorite-priority updates.

🤖 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.

Outside diff comments:
In
`@core/service/src/androidMain/kotlin/org/meshtastic/core/service/ConversationShortcutPublisher.kt`:
- Around line 156-160: Update the label fallback logic in
ConversationShortcutPublisher so shortLabel and longLabel use the existing
localized generic fallback when node metadata is missing, rather than
dm.contactKey. Keep contactKey restricted to internal identifiers and deep-link
metadata, and flag any leftover placeholders or logging that exposes it.
- Line 193: Replace the hardcoded fallback in the channelName assignment with
the localized channel-label resource from core:resources, loading it through the
project’s established nonblocking resource-access path and passing channel.index
as the format argument. Preserve channel.name when non-empty.
- Around line 137-140: Update publishShortcuts cleanup to avoid removing
on-demand shortcuts while the contacts snapshot is incomplete. Restrict stale-ID
pruning to shortcuts owned by the publisher, and preserve shortcuts created by
ensureConversationShortcut until the observed contacts flow includes their
conversation; apply the same ownership and preservation logic to the cleanup
path around the additional referenced block.

---

Nitpick comments:
In
`@core/service/src/androidMain/kotlin/org/meshtastic/core/service/ConversationShortcutPublisher.kt`:
- Around line 81-85: Update the shortcut-publishing flow around
rankConversationsByRecency to also collect a node-database or version trigger
from nodeRepository, so buildDmShortcut-derived labels, icons, and favorite
priority refresh when node metadata changes. Preserve the existing contacts and
channel-set inputs, and add regression coverage for node rename and
favorite-priority updates.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: a163356b-a04f-41e2-b8e4-7c068a8ea60e

📥 Commits

Reviewing files that changed from the base of the PR and between 220ccc8 and d7cff2d.

📒 Files selected for processing (5)

• core/service/src/androidHostTest/kotlin/org/meshtastic/core/service/ConversationShortcutPublisherTest.kt
• core/service/src/androidHostTest/kotlin/org/meshtastic/core/service/MeshNotificationManagerImplConversationTest.kt
• core/service/src/androidMain/kotlin/org/meshtastic/core/service/ConversationShortcutPublisher.kt
• core/service/src/androidMain/kotlin/org/meshtastic/core/service/MeshNotificationManagerImpl.kt
• core/service/src/androidMain/kotlin/org/meshtastic/core/service/ReplyReceiver.kt

🚧 Files skipped from review as they are similar to previous changes (4)

• core/service/src/androidHostTest/kotlin/org/meshtastic/core/service/ConversationShortcutPublisherTest.kt
• core/service/src/androidMain/kotlin/org/meshtastic/core/service/ReplyReceiver.kt
• core/service/src/androidHostTest/kotlin/org/meshtastic/core/service/MeshNotificationManagerImplConversationTest.kt
• core/service/src/androidMain/kotlin/org/meshtastic/core/service/MeshNotificationManagerImpl.kt