feat: detect admin/dispute DMs in background notification pipeline#498
Open
AndreaDiazCorreia wants to merge 10 commits intomainfrom
Open
feat: detect admin/dispute DMs in background notification pipeline#498AndreaDiazCorreia wants to merge 10 commits intomainfrom
AndreaDiazCorreia wants to merge 10 commits intomainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughDetects DM (admin/dispute) envelopes using Changes
Sequence Diagram(s)sequenceDiagram
participant Event as Nostr Event
participant Decrypt as Decryption
participant JSON as JSON Decode
participant Detect as NostrUtils.isDmPayload
participant Router as Background Service
participant Store as EventStore
participant Mostro as MostroService
participant Notif as NotificationExtractor
Event->>Decrypt: decrypt content
Decrypt->>JSON: decode -> list
JSON->>Detect: check first item
Detect-->>Router: is DM?
alt DM payload
Router->>Mostro: build MostroMessage(action=sendDm, id=orderId, ts=createdAt)
Mostro->>Notif: dispatch MostroMessage
Notif->>Notif: extract NotificationData (sendDm)
else Non-DM payload
Router->>Store: reserve event.id
Store-->>Mostro: stored/reserved
Mostro->>Notif: MostroMessage.fromJson -> process normally
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
lib/features/notifications/services/background_notification_service.dart
Outdated
Show resolved
Hide resolved
![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.
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
AndreaDiazCorreia
force-pushed
the
feat/admin-chat-background-notifications
branch
from
c9cc1d2 to
63486cf
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/services/mostro_service.dart (1)
133-137: Wording mismatch: this branch skips allsend-dmpayloads, not only dispute chat.The behavior is broader than the current comment/log text, which can confuse incident triage for admin DMs.
Suggested wording tweak
- // Skip dispute chat messages before reserving in eventStorage - // so DisputeChatNotifier can still process them + // Skip DM envelope payloads (e.g., dispute/admin send-dm) before reserving + // so dedicated DM handlers can process them without this pipeline consuming them if (NostrUtils.isDmPayload(result[0])) { - logger.i('Skipping dispute chat message (handled by DisputeChatNotifier)'); + logger.i('Skipping send-dm payload in MostroService (handled by DM-specific handlers)'); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/mostro_service.dart` around lines 133 - 137, The comment and log message incorrectly claim only "dispute chat" messages are skipped; update the comment above the NostrUtils.isDmPayload(result[0]) branch and the logger.i call to reflect that all send-dm / direct message payloads are being skipped (not just dispute chat), e.g., mention "direct messages (send-dm) are skipped so DisputeChatNotifier or DM handler can process them" and update the logger.i text to the same clearer wording; change references around NostrUtils.isDmPayload and DisputeChatNotifier accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/services/mostro_service.dart`:
- Around line 133-137: The comment and log message incorrectly claim only
"dispute chat" messages are skipped; update the comment above the
NostrUtils.isDmPayload(result[0]) branch and the logger.i call to reflect that
all send-dm / direct message payloads are being skipped (not just dispute chat),
e.g., mention "direct messages (send-dm) are skipped so DisputeChatNotifier or
DM handler can process them" and update the logger.i text to the same clearer
wording; change references around NostrUtils.isDmPayload and DisputeChatNotifier
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2777a7d9-38ff-4d85-9d97-1a1974256c1d
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
lib/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
✅ Files skipped from review due to trivial changes (2)
- lib/shared/utils/nostr_utils.dart
- test/features/notifications/services/background_notification_dm_detection_test.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/features/notifications/utils/notification_data_extractor.dart
- lib/features/notifications/services/background_notification_service.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/notifications/utils/notification_message_mapper.dart`:
- Line 73: Add localization resolution for the new admin DM keys by updating the
_resolveLocalizationKey function to handle the new keys (e.g.,
'notification_admin_message_title' and the matching body key returned at line
169) and return S.of(context)!.notificationAdminMessageTitle and
S.of(context)!.notificationAdminMessageBody (use the exact generated S property
names matching your arb keys). Also add the import for
package:mostro_mobile/generated/l10n.dart at the top of the file so
S.of(context)! is available. Ensure the keys in _resolveLocalizationKey match
the string constants used in the mapper so notifications show translated text
instead of raw keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ff74414-cdb2-4135-b667-66e6e97f140f
📒 Files selected for processing (6)
lib/features/notifications/utils/notification_message_mapper.dartlib/l10n/intl_de.arblib/l10n/intl_en.arblib/l10n/intl_es.arblib/l10n/intl_fr.arblib/l10n/intl_it.arb
✅ Files skipped from review due to trivial changes (5)
- lib/l10n/intl_en.arb
- lib/l10n/intl_es.arb
- lib/l10n/intl_de.arb
- lib/l10n/intl_fr.arb
- lib/l10n/intl_it.arb
Member
Author
Contributor
There was a problem hiding this comment.
Strict Review: Admin/Dispute DM Background Notifications
⚠️ CRITICAL ISSUES (Must Fix)
1. Race Condition in Event Storage (HIGH SEVERITY)
File: lib/services/mostro_service.dart
Lines: 108-143
// BEFORE (good):
if (await eventStore.hasItem(event.id!)) return;
await eventStore.putItem(event.id!, {...});
// AFTER (race condition):
if (await eventStore.hasItem(event.id!)) return;
// ... processing ...
if (NostrUtils.isDmPayload(result[0])) {
return; // Event NOT stored
}
await eventStore.putItem(event.id!, {...}); // Stored AFTER processingProblem:
You moved eventStore.putItem() after the DM payload check. This creates a race condition:
- Event arrives
hasItem()returns false (event not seen)- Decrypt event
- Check
isDmPayload()→ true - Return early WITHOUT storing event ID
- Same event arrives again from another relay
hasItem()still false → duplicate processing
Impact:
- Admin DMs processed multiple times (duplicate notifications)
- DisputeChatNotifier also processes the same event
- Potential notification spam
Fix:
Store event ID immediately after hasItem() check, BEFORE any processing:
if (await eventStore.hasItem(event.id!)) return;
// Reserve event ID FIRST (atomic claim)
await eventStore.putItem(event.id!, {
'id': event.id,
'created_at': event.createdAt!.millisecondsSinceEpoch ~/ 1000,
});
// Then process
final decryptedEvent = await event.unWrap(privateKey);
// ... rest of logic2. Insufficient Validation in isDmPayload() (MEDIUM SEVERITY)
File: lib/shared/utils/nostr_utils.dart
Lines: 450-453
static bool isDmPayload(dynamic item) {
if (item is! Map) return false;
final dm = item['dm'];
return dm is Map && dm['action'] == 'send-dm';
}Problems:
-
No validation that
dmMap is non-empty{'dm': {}}returns false (good), but{'dm': {'action': null}}would fail later
-
No validation of payload structure
- What if
dm['payload']is malformed? - What if
dm['text_message']is missing?
- What if
Test Gap:
Your tests check {'dm': {}} (empty) and {'dm': 'not-a-map'}, but NOT:
{'dm': {'action': 'send-dm', 'payload': null}}{'dm': {'action': 'send-dm', 'payload': {'malformed': 'data'}}}
Fix:
static bool isDmPayload(dynamic item) {
if (item is! Map) return false;
final dm = item['dm'];
if (dm is! Map) return false;
// Validate action AND payload presence
if (dm['action'] != 'send-dm') return false;
final payload = dm['payload'];
if (payload is! Map) return false;
return true; // Valid DM structure
}Add test:
test('validates dm payload is a Map', () {
expect(NostrUtils.isDmPayload({'dm': {'action': 'send-dm', 'payload': null}}), isFalse);
expect(NostrUtils.isDmPayload({'dm': {'action': 'send-dm', 'payload': []}}), isFalse);
});3. No Error Handling for JSON Parse in _onNotificationTap (MEDIUM SEVERITY)
File: lib/features/notifications/services/background_notification_service.dart
Lines: 78-106
try {
final data = jsonDecode(payload) as Map<String, dynamic>;
final type = data['type'] as String?;
final orderId = data['orderId'] as String?;
// ...
} catch (_) {
// Not JSON — treat as plain orderId (legacy format)
}Problem:
The catch block swallows ALL exceptions, not just JSON parse errors:
jsonDecode()throwsFormatExceptionfor invalid JSON ✅- But
as Map<String, dynamic>throwsTypeErrorif JSON is not a Map ❌ - And
data['type'] as String?can throw iftypeexists but is not String ❌
Fix:
Catch specific exceptions and log unexpected errors:
try {
final data = jsonDecode(payload);
if (data is! Map<String, dynamic>) {
throw FormatException('Payload is not a JSON object');
}
final type = data['type'];
if (type is! String) {
throw FormatException('Invalid type field');
}
final orderId = data['orderId'];
if (orderId is! String) {
throw FormatException('Invalid orderId field');
}
// ... rest of logic
} on FormatException catch (e) {
// Expected: malformed JSON or wrong structure
logger.d('Payload is not JSON, treating as plain orderId: $e');
} catch (e, stack) {
// Unexpected error — log it
logger.e('Unexpected error parsing notification payload', error: e, stackTrace: stack);
}⚠️ DESIGN ISSUES (Should Fix)
4. Brittle Fallback Logic for Dispute ID Lookup (MEDIUM SEVERITY)
File: lib/features/notifications/services/background_notification_service.dart
Lines: 84-97
try {
final container = ProviderScope.containerOf(context);
final orderState = container.read(orderNotifierProvider(orderId));
final disputeId = orderState.dispute?.disputeId;
if (disputeId != null) {
context.push('/dispute_details/$disputeId');
return;
}
} catch (e) {
logger.w('Could not read dispute state, falling back to trade detail: $e');
}Problems:
-
Assumes app is in foreground
ProviderScope.containerOf(context)can fail if app was terminated- Order state might not be loaded yet
-
Silent fallback hides real errors
- User taps "Admin sent you a message" → lands on trade detail instead of dispute chat
- No indication that navigation failed
-
Why not include disputeId in the payload?
- You're already encoding
orderIdin JSON - Session has
orderId→ you can look updisputeIdin background service
- You're already encoding
Recommendation:
Include disputeId in notification payload when creating the notification:
// In showLocalNotification():
final notificationPayload = notificationData.action == Action.sendDm
? jsonEncode({
'type': 'admin_dm',
'orderId': mostroMessage.id,
'disputeId': session.dispute?.disputeId, // Include if available
})
: mostroMessage.id;Then in _onNotificationTap:
final disputeId = data['disputeId'] as String?;
if (type == 'admin_dm') {
if (disputeId != null) {
context.push('/dispute_details/$disputeId');
} else {
// Fallback: no disputeId in payload
context.push('/trade_detail/$orderId');
}
return;
}Why better:
- No runtime dependency on order state being loaded
- Works even if app was killed and restarted
- Clearer error path
📝 NITPICKS (Nice to Fix)
5. Inconsistent Error Logging
File: lib/features/notifications/services/background_notification_service.dart
- Line 201:
logger.w('DM received but session has no orderId, skipping notification') - Line 257:
logger.w('Admin DM received but session has no orderId, skipping notification')
Both log the same condition but from different code paths. Consider:
logger.w('DM received for session without orderId (recipient: ${event.recipient}), skipping notification');Including event.recipient helps debugging which session had the issue.
6. Missing Test: disputeId Navigation
You test that admin DM notifications are created, but NOT that tapping them navigates correctly.
Add integration test:
test('admin DM notification navigates to dispute chat when disputeId available', () async {
// Setup: mock notification with disputeId
final payload = jsonEncode({
'type': 'admin_dm',
'orderId': 'order-123',
'disputeId': 'dispute-456',
});
// Simulate tap
_onNotificationTap(NotificationResponse(
notificationResponseType: NotificationResponseType.selectedNotification,
payload: payload,
));
// Verify navigation
expect(find.byRoute('/dispute_details/dispute-456'), findsOneWidget);
});✅ WHAT'S GOOD
- Comprehensive test coverage — You test all 3 layers (detection, construction, extraction)
- Clear separation of concerns —
NostrUtils.isDmPayload()is reusable - Non-invasive — Uses existing notification pipeline with minimal changes
- Proper use of NIP-40 — Dispute chat subscription via
adminSharedKey
Summary
Must Fix (Blocking Merge)
- Fix race condition in event storage (move
putItembefore processing) - Strengthen
isDmPayload()validation (check payload structure) - Add specific exception handling in
_onNotificationTap(don't swallow TypeErrors)
Should Fix (Recommended)
- Include disputeId in notification payload (remove runtime dependency on order state)
- Add test for dispute navigation
- Improve error logging (include recipient in DM skip warnings)
Code Quality
✅ Tests pass
✅ No obvious type errors
Verdict: Request Changes — Fix the race condition and validation issues, then I'll approve.
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