feat: detect admin/dispute DMs in background notification pipeline#498
Open
AndreaDiazCorreia wants to merge 6 commits intomainfrom
Open
feat: detect admin/dispute DMs in background notification pipeline#498AndreaDiazCorreia wants to merge 6 commits intomainfrom
AndreaDiazCorreia wants to merge 6 commits intomainfrom
Conversation
Add detection for admin/dispute DM messages in background service by checking
for {"dm": {...}} format before standard MostroMessage parsing. Return synthetic
MostroMessage with sendDm action to trigger notification flow.
Part of chat notifications implementation (Phase 1: Admin DM background notifications).
…load
Extract duplicate DM format detection logic (`item is Map && item.containsKey('dm')`)
into shared `NostrUtils.isDmPayload()` method. Replace inline checks in
DisputeChatNotifier, BackgroundNotificationService, and MostroService with calls
to the new utility.
Update tests to exercise NostrUtils.isDmPayload directly instead of testing
detection logic in isolation. Add edge case coverage for non-Map types.
…xtractor Add explicit case handlers for Action.sendDm and Action.cooperativeCancelAccepted in NotificationDataExtractor to ensure they generate non-temporary notifications. Both actions require no payload extraction (empty values map). Expand test coverage to validate three layers of the admin/dispute DM notification pipeline: NostrUtils.isDmPayload detection, MostroMessage construction with sendDm action, and NotificationDataExtractor
Contributor
WalkthroughAdds DM-payload detection and handling across message processing: a new NostrUtils.isDmPayload utility, DM-aware parsing in dispute chat notifier and background notification service, and early-skip for DM payloads in mostro_service; tests added for DM detection and extractor behavior. Changes
Sequence DiagramsequenceDiagram
participant NotifSystem as Notification System
participant BGService as BackgroundNotificationService
participant NostrUtils as NostrUtils
participant MostroService as MostroService
participant DisputeNotifier as DisputeChatNotifier
NotifSystem->>BGService: Receive encrypted event (kind 1059)
BGService->>BGService: Decrypt payload -> parse first item
BGService->>NostrUtils: isDmPayload(firstItem)?
alt DM payload
NostrUtils-->>BGService: true
BGService->>BGService: Build MostroMessage(action: sendDm)
BGService-->>NotifSystem: Emit DM MostroMessage
NotifSystem->>MostroService: Deliver message
MostroService->>NostrUtils: isDmPayload?
NostrUtils-->>MostroService: true
MostroService->>MostroService: Log & skip further handling
NotifSystem->>DisputeNotifier: New chat event (1059)
DisputeNotifier->>DisputeNotifier: mostroUnWrap using tradeKey
DisputeNotifier->>DisputeNotifier: Parse unwrapped content (DM JSON or plain)
DisputeNotifier->>DisputeNotifier: Extract messageText, senderPubkey, isFromAdmin
DisputeNotifier->>DisputeNotifier: Deduplicate & store
else Standard payload
NostrUtils-->>BGService: false
BGService->>BGService: Parse as standard MostroMessage
BGService-->>NotifSystem: Emit standard MostroMessage
NotifSystem->>MostroService: Deliver message
MostroService->>MostroService: Restore-payload handling
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/features/notifications/services/background_notification_dm_detection_test.dart (1)
1-134: Consider addingNotificationMessageMapperlayer tests to validate the full background notification pipeline.The three pipeline layers tested here (detection → construction → extraction) are covered, but the fourth layer —
NotificationMessageMapper.getLocalizedTitleWithInstance/getLocalizedMessageWithInstance— executes in_getLocalizedNotificationTextafter extraction and before the notification is displayed. While the mapper already has entries forAction.sendDmandAction.cooperativeCancelAccepted, adding a test that calls the mapper directly with a concreteSinstance (e.g.SEn()) for these actions would confirm that localization keys resolve correctly and close this coverage gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/features/notifications/services/background_notification_dm_detection_test.dart` around lines 1 - 134, Add tests that exercise NotificationMessageMapper.getLocalizedTitleWithInstance and getLocalizedMessageWithInstance for the actions covered (Action.sendDm and Action.cooperativeCancelAccepted) using a concrete localization instance (e.g., SEn) to ensure localization keys resolve; create test cases that build a NotificationData (or MostroMessage -> extract via NotificationDataExtractor) and then call NotificationMessageMapper.getLocalizedTitleWithInstance(SEn()) and getLocalizedMessageWithInstance(SEn()) asserting non-empty/expected strings, mirroring existing test patterns in this file and referencing NotificationMessageMapper, getLocalizedTitleWithInstance, getLocalizedMessageWithInstance, and SEn to locate where to add the new tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/notifications/services/background_notification_service.dart`:
- Around line 175-183: The code constructs a MostroMessage using
matchingSession.orderId which can be null and results in a null notification
payload; add an explicit guard: check matchingSession.orderId before building
the MostroMessage in background_notification_service.dart (the block that
returns MostroMessage for NostrUtils.isDmPayload firstItem), and if orderId is
null either early-return/skip creating the DM notification or set a non-null
fallback (e.g., "unlinked-session") for MostroMessage.id so
flutterLocalNotificationsPlugin.show never receives a null payload; ensure the
chosen approach is clearly documented in the conditional.
---
Nitpick comments:
In
`@test/features/notifications/services/background_notification_dm_detection_test.dart`:
- Around line 1-134: Add tests that exercise
NotificationMessageMapper.getLocalizedTitleWithInstance and
getLocalizedMessageWithInstance for the actions covered (Action.sendDm and
Action.cooperativeCancelAccepted) using a concrete localization instance (e.g.,
SEn) to ensure localization keys resolve; create test cases that build a
NotificationData (or MostroMessage -> extract via NotificationDataExtractor) and
then call NotificationMessageMapper.getLocalizedTitleWithInstance(SEn()) and
getLocalizedMessageWithInstance(SEn()) asserting non-empty/expected strings,
mirroring existing test patterns in this file and referencing
NotificationMessageMapper, getLocalizedTitleWithInstance,
getLocalizedMessageWithInstance, and SEn to locate where to add the new tests.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/features/disputes/notifiers/dispute_chat_notifier.dartlib/features/notifications/services/background_notification_service.dartlib/features/notifications/utils/notification_data_extractor.dartlib/services/mostro_service.dartlib/shared/utils/nostr_utils.darttest/features/notifications/services/background_notification_dm_detection_test.dart
Comment on lines
+175
to
+183
| // Detect admin/dispute DM format: [{"dm": {"action": "send-dm", ...}}] | ||
| final firstItem = result[0]; | ||
| if (NostrUtils.isDmPayload(firstItem)) { | ||
| return MostroMessage( | ||
| action: mostro_action.Action.sendDm, | ||
| id: matchingSession.orderId, | ||
| timestamp: event.createdAt?.millisecondsSinceEpoch, | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
matchingSession.orderId null produces a null notification payload.
matchingSession.orderId is String?. If it is null (unlinked session), MostroMessage.id is null, so payload: mostroMessage.id passed to flutterLocalNotificationsPlugin.show is also null. Tapping the notification navigates to /notifications rather than /trade_detail/$orderId.
In practice dispute sessions are always linked to an order, so this is low-risk, but an explicit guard here makes the intent clear.
🛡️ Proposed defensive guard
if (NostrUtils.isDmPayload(firstItem)) {
+ if (matchingSession.orderId == null) {
+ logger.w('DM payload detected but session has no orderId — notification will lack deep-link');
+ }
return MostroMessage(
action: mostro_action.Action.sendDm,
id: matchingSession.orderId,
timestamp: event.createdAt?.millisecondsSinceEpoch,
);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/features/notifications/services/background_notification_service.dart`
around lines 175 - 183, The code constructs a MostroMessage using
matchingSession.orderId which can be null and results in a null notification
payload; add an explicit guard: check matchingSession.orderId before building
the MostroMessage in background_notification_service.dart (the block that
returns MostroMessage for NostrUtils.isDmPayload firstItem), and if orderId is
null either early-return/skip creating the DM notification or set a non-null
fallback (e.g., "unlinked-session") for MostroMessage.id so
flutterLocalNotificationsPlugin.show never receives a null payload; ensure the
chosen approach is clearly documented in the conditional.
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code looks solid — clean extraction of isDmPayload(), good test coverage, and the synthetic MostroMessage approach for DM detection is the right call.
However, this PR has merge conflicts with the base branch (mergeable_state: dirty). The conflict likely comes from recent changes in dispute_chat_notifier.dart (PR #501 refactored the same file significantly).
Please rebase/merge from main to resolve conflicts, then this should be good to go.
Code review notes (all positive):
NostrUtils.isDmPayload()is a clean DRY improvement over the 3 inline checkssendDmandcooperativeCancelAcceptedcases inNotificationDataExtractorcorrectly marked as non-temporary- Tests cover detection, construction, and extraction layers thoroughly
- The early return before
MostroMessage.fromJson()in background service is the right place to intercept
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/disputes/notifiers/dispute_chat_notifier.dart (1)
250-263:⚠️ Potential issue | 🔴 CriticalRemove the old p2p-unwrapping block.
The stale branch below re-declares
unwrappedEvent,messageText, andisFromAdmin, which is why the analyzer fails at Lines 251, 256, and 262. It also bypasses the newly parsed DM text and would keep rendering the raw inner JSON instead of the extracted chat message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/disputes/notifiers/dispute_chat_notifier.dart` around lines 250 - 263, Remove the stale p2p-unwrapping block that re-declares unwrappedEvent, messageText, and isFromAdmin (the code that calls event.p2pUnwrap(session.adminSharedKey!), reassigns unwrappedEvent.content to messageText, and computes isFromAdmin against session.tradeKey.public), since it shadows the newly parsed DM and causes analyzer errors and renders raw JSON; instead keep and use the previously parsed DM/unwrappedEvent and the existing DisputeChatMessage(event: unwrappedEvent) path that relies on session.adminSharedKey and session.tradeKey.public.
🧹 Nitpick comments (1)
test/features/notifications/services/background_notification_dm_detection_test.dart (1)
65-133: Cover the real background-service branch, not just re-created objects.This suite never exercises
_decryptAndProcessEvent()or an extracted helper, so it will not catch regressions in the actual JSON decode / DM detection /orderIdmapping path changed by this PR. A small test seam around that branch would protect the shipped behavior much better.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/features/notifications/services/background_notification_dm_detection_test.dart` around lines 65 - 133, Add a test that exercises the real background-service branch by invoking the actual decryption/processing path instead of only constructing MostroMessage objects: call the public entrypoint that leads to _decryptAndProcessEvent (or, if needed for tests, expose a test-only wrapper around _decryptAndProcessEvent) with a MostroMessage containing an encrypted/payload JSON matching the real background-service format, then assert the resulting NotificationData (from NotificationDataExtractor.extractFromMostroMessage or the return of _decryptAndProcessEvent) preserves orderId, detects sendDm correctly, and maps values as in production; this will ensure the JSON decode/DM detection/orderId mapping changes are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/disputes/notifiers/dispute_chat_notifier.dart`:
- Around line 170-225: Add the missing imports for the new types and provider
used in dispute_chat_notifier.dart so the parser path compiles: import the
provider that exposes disputeDetailsProvider and the model definitions for
MostroMessage, Action, and TextMessage (the files that declare those symbols)
and ensure they are referenced at top of the file so
disputeDetailsProvider(disputeId), MostroMessage.fromJson, Action.sendDm, and
MostroMessage.getPayload<TextMessage>() resolve correctly.
- Around line 165-177: The handler unwrapped events with session.tradeKey
(event.mostroUnWrap using session.tradeKey) but the surrounding flow still uses
adminSharedKey in _subscribe(), _listenForSession(), and
_loadHistoricalMessages(), so new NIP-59 admin DMs encrypted to the trade key
are never seen; update those three functions to subscribe, filter, and decrypt
using the session.tradeKey (or accept both adminSharedKey and session.tradeKey
for backwards compatibility), ensure subscription filters/registers use
session.tradeKey, replace decryption calls that use adminSharedKey with the
trade key (or try trade key first then fall back to adminSharedKey), and make
sure _listenForSession() and _loadHistoricalMessages() pass the session/tradeKey
into any downstream decrypt/unwrap logic so unwrappedEvent processing matches
the subscription keys.
In `@lib/services/mostro_service.dart`:
- Around line 133-137: The DM-guard is running after the code that
reserves/inserts event.id into the shared eventStorageProvider, causing DMs to
be marked seen before DisputeChatNotifier._onChatEvent can handle them; move the
NostrUtils.isDmPayload(result[0]) check to run before any reservation/insertion
of event.id (i.e., before the logic that writes to eventStorageProvider /
reserves the id inside the MostroService method containing this block) so that
DMs are returned early and delegated to DisputeChatNotifier without touching the
shared store.
---
Outside diff comments:
In `@lib/features/disputes/notifiers/dispute_chat_notifier.dart`:
- Around line 250-263: Remove the stale p2p-unwrapping block that re-declares
unwrappedEvent, messageText, and isFromAdmin (the code that calls
event.p2pUnwrap(session.adminSharedKey!), reassigns unwrappedEvent.content to
messageText, and computes isFromAdmin against session.tradeKey.public), since it
shadows the newly parsed DM and causes analyzer errors and renders raw JSON;
instead keep and use the previously parsed DM/unwrappedEvent and the existing
DisputeChatMessage(event: unwrappedEvent) path that relies on
session.adminSharedKey and session.tradeKey.public.
---
Nitpick comments:
In
`@test/features/notifications/services/background_notification_dm_detection_test.dart`:
- Around line 65-133: Add a test that exercises the real background-service
branch by invoking the actual decryption/processing path instead of only
constructing MostroMessage objects: call the public entrypoint that leads to
_decryptAndProcessEvent (or, if needed for tests, expose a test-only wrapper
around _decryptAndProcessEvent) with a MostroMessage containing an
encrypted/payload JSON matching the real background-service format, then assert
the resulting NotificationData (from
NotificationDataExtractor.extractFromMostroMessage or the return of
_decryptAndProcessEvent) preserves orderId, detects sendDm correctly, and maps
values as in production; this will ensure the JSON decode/DM detection/orderId
mapping changes are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6c1c48b-d97d-45d2-887f-256bd302d862
📒 Files selected for processing (6)
lib/features/disputes/notifiers/dispute_chat_notifier.dartlib/features/notifications/services/background_notification_service.dartlib/features/notifications/utils/notification_data_extractor.dartlib/services/mostro_service.dartlib/shared/utils/nostr_utils.darttest/features/notifications/services/background_notification_dm_detection_test.dart
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/disputes/notifiers/dispute_chat_notifier.dart`:
- Around line 203-225: The code decodes JSON into messageText but still uses the
original unwrappedEvent when adding messages, causing the UI to display raw DM
payloads and bypassing TextMessage parsing; create or reuse a shared helper
(e.g., normalizeDmBody/unpackDmPayload) that takes the unwrappedEvent content
and returns the canonical text body by handling both "message" and
"text_message" keys and other DM payload shapes (use NostrUtils.isDmPayload to
detect DM payloads), then call that helper here (in dispute_chat_notifier.dart)
and in _loadHistoricalMessages() and ensure the code uses the helper's
normalized string when constructing messages/state instead of wrapping/passing
the original unwrappedEvent JSON.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bacf0621-bb3a-4e9c-b2d5-8cd70e9dca84
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
lib/features/disputes/notifiers/dispute_chat_notifier.dart
Contributor
There was a problem hiding this comment.
Thanks for the implementation work — the direction makes sense, but I’m requesting changes due to one blocker and a couple of robustness gaps.
Blocking issue
1) DM payload detection is too permissive (NostrUtils.isDmPayload)
Current logic only checks whether the decoded item is a Map containing a dm key.
That can classify non-DM payloads as Action.sendDm in background processing, because _decryptAndProcessEvent converts any isDmPayload(...) == true case into a synthetic MostroMessage(sendDm, ...).
Please tighten detection to validate shape, e.g.:
item is Mapitem['dm'] is Mapitem['dm']['action'] == 'send-dm'(string)
Optionally validate the expected nested fields for dispute/admin DM payloads.
Non-blocking but important
2) Add negative tests for malformed/partial DM wrappers
The new test suite is good, but it misses key negative cases:
{'dm': 'not-a-map'}{'dm': {}}without action{'dm': {'action': 'different-action'}}
These should return false from isDmPayload to prevent false-positive notification routing.
3) Keep foreground/background classification aligned
Please ensure DM classification semantics are identical across foreground and background paths to avoid divergence in notification behavior.
Once the DM classifier is strict and tests cover malformed wrappers, this should be much safer to merge.
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.
{"dm": ...}format) in the background notification serviceand display notifications instead of silently failing
NostrUtils.isDmPayload()utility to replace duplicated DM detection logicacross 3 files
sendDmandcooperativeCancelAcceptedcases toNotificationDataExtractorsothey generate proper non-temporary notifications
Context
Admin/dispute chat messages arrive at
tradeKey.publicand get decrypted successfully byunWrap(), butMostroMessage.fromJson()fails because the inner format is[{"dm": {...}}]instead of the standard Mostro message format. This caused background notifications for admin DMs
to be silently dropped.
This PR (Phase 1 of the chat notifications plan) fixes this by
detecting the DM format before JSON parsing and constructing a synthetic
MostroMessagewithAction.sendDmthat flows through the existing notification pipeline.Changes
lib/shared/utils/nostr_utils.dartisDmPayload()static methodlib/features/notifications/services/background_notification_service.dartMostroMessage.fromJsonlib/features/notifications/utils/notification_data_extractor.dartsendDm+cooperativeCancelAcceptedcaseslib/services/mostro_service.dartisDmPayload()lib/features/disputes/notifiers/dispute_chat_notifier.dartisDmPayload()How to test
flutter test test/features/notifications/services/background_notification_dm_detection_test.dart/dm <order_id> <message>)cancellations, timeouts
Summary by CodeRabbit
New Features
Bug Fixes
Tests