Add P2P chat background notifications (Phase 2)#529
Add P2P chat background notifications (Phase 2)#529AndreaDiazCorreia wants to merge 10 commits intomainfrom
Conversation
|
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:
WalkthroughReworks background notification decryption: early-return when recipient is null, route events by matching recipient against session tradeKey.public or sharedKey.public, and extract chat DM vs P2P chat via new Changes
Sequence Diagram(s)mermaid 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 |
…n DM and protocol messages
AndreaDiazCorreia
force-pushed
the
feat/p2p-chat-background-notifications
branch
from
c0bc2b6 to
ac3cfbe
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 293-310: The _handleP2PChatEvent function currently builds a
MostroMessage using session.orderId without checking for null; update
_handleP2PChatEvent to validate session.orderId != null (mirroring the pattern
in _processAdminDm and _handleTradeKeyEvent), and if it's null log a warning
(use the same logger) and return null instead of creating the sendDm message to
avoid creating notifications before orderId is populated.
- Around line 221-225: Add a null-check for session.orderId in
_handleP2PChatEvent before constructing the MostroMessage: if session.orderId is
null, log a warning (include event.recipient or other context) and return null
to avoid creating a message with id: null; mirror the existing checks in
_processAdminDm and _handleTradeKeyEvent so the function only builds and returns
a MostroMessage when session.orderId is non-null.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6a7334b9-d14c-4d35-a1bb-86a7f62e1378
📒 Files selected for processing (1)
lib/features/notifications/services/background_notification_service.dart
lib/features/notifications/services/background_notification_service.dart
Outdated
Show resolved
Hide resolved
lib/features/notifications/services/background_notification_service.dart
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/features/notifications/services/background_notification_service.dart (1)
261-291: Add try-catch for consistency with sibling handlers.Unlike
_processAdminDm(lines 255-258) and_handleP2PChatEvent(lines 311-314), this function lacks its own try-catch block. While the outer_decryptAndProcessEventcatch block will handle exceptions, the inconsistent pattern makes debugging harder—exceptions from this function will log as generic "Decrypt error" rather than identifying the specific handler that failed.Additionally,
result[0]passed toMostroMessage.fromJsonon line 287 is typed asdynamicand may not be aMap<String, dynamic>for malformed payloads.♻️ Proposed fix to add error handling
/// Handle events matched by tradeKey (Mostro protocol + admin/dispute DMs) Future<MostroMessage?> _handleTradeKeyEvent(NostrEvent event, Session session) async { + try { final decryptedEvent = await event.unWrap(session.tradeKey.private); if (decryptedEvent.content == null) { return null; } final result = jsonDecode(decryptedEvent.content!); if (result is! List || result.isEmpty) { return null; } // Detect admin/dispute DM format that arrived via tradeKey final firstItem = result[0]; if (NostrUtils.isDmPayload(firstItem)) { if (session.orderId == null) { logger.w('DM received but session has no orderId (recipient: ${event.recipient}), skipping notification'); return null; } return MostroMessage( action: mostro_action.Action.sendDm, id: session.orderId, timestamp: event.createdAt?.millisecondsSinceEpoch, ); } + if (result[0] is! Map<String, dynamic>) { + logger.w('Unexpected payload format in tradeKey event'); + return null; + } final mostroMessage = MostroMessage.fromJson(result[0]); mostroMessage.timestamp = event.createdAt?.millisecondsSinceEpoch; return mostroMessage; + } catch (e) { + logger.e('Trade key decrypt error: $e'); + return null; + } }🤖 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 261 - 291, _handleTradeKeyEvent lacks its own try/catch and performs an unsafe cast when calling MostroMessage.fromJson(result[0]); wrap the body of _handleTradeKeyEvent in a try/catch, catch (e, st) and log a handler-specific error via logger.e (include the exception and stack) and return null on error; before calling MostroMessage.fromJson, validate that result[0] is a Map<String, dynamic> (or safely convert) and if not, log a warning and return null; keep existing behavior for DM detection using NostrUtils.isDmPayload and preserve timestamp assignment on the MostroMessage instance.
🤖 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/features/notifications/services/background_notification_service.dart`:
- Around line 261-291: _handleTradeKeyEvent lacks its own try/catch and performs
an unsafe cast when calling MostroMessage.fromJson(result[0]); wrap the body of
_handleTradeKeyEvent in a try/catch, catch (e, st) and log a handler-specific
error via logger.e (include the exception and stack) and return null on error;
before calling MostroMessage.fromJson, validate that result[0] is a Map<String,
dynamic> (or safely convert) and if not, log a warning and return null; keep
existing behavior for DM detection using NostrUtils.isDmPayload and preserve
timestamp assignment on the MostroMessage instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a0a6ef65-b28f-4588-9ca7-3bc4498ce338
📒 Files selected for processing (1)
lib/features/notifications/services/background_notification_service.dart
|
Thanks for the update — I reviewed the full diff in I found one blocking behavior regression:
Why this mattersThis PR introduces P2P chat background notifications by reusing the same Suggested fixUse separate handling for P2P chat vs admin dispute DMs (either a distinct action/key path, or an explicit metadata flag that reaches
Also recommended before mergeAdd focused tests for this branch:
Once that separation is in place, the structural refactor here looks good. |
Introduce Action.chatMessage to distinguish P2P chat notifications from admin/dispute DMs (Action.sendDm). Update background notification pipeline to use chatMessage for P2P events, add notification string mappings, and route to trade detail screen. Expand test coverage for action separation and notification data extraction.
|
Great progress on this PR — the In Right now Why this mattersEven if build/tests pass, this can cause misclassification in notification UI behavior (filtering/grouping/icon logic tied to type). Suggested fixAdd After this is addressed, this looks ready to approve from my side. |
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)
254-309: Exercise the newsharedKey.publicdecrypt path directly.These assertions start after a
MostroMessagealready exists, so a regression inbackground_notification_service.dart's new shared-key match orp2pUnwrap(session.sharedKey!)branch would still pass. Please add one test that feeds a matching shared-key event through the background pipeline and asserts it emitsAction.chatMessage.🤖 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 254 - 309, Add a test that constructs a MostroMessage whose payload is encrypted for the test session's sharedKey.public and then runs it through the background pipeline to ensure the p2pUnwrap(session.sharedKey!) branch is exercised: create or reuse a test Session with a matching sharedKey.public, build an encrypted MostroMessage that would decrypt to Action.chatMessage, call the background pipeline entrypoint (BackgroundNotificationService.processIncoming / handleMostroMessage or equivalent used in your codebase) and assert the emitted/produced NotificationData has action == Action.chatMessage; this ensures the new shared-key decrypt path (p2pUnwrap) is exercised rather than bypassed by directly calling NotificationDataExtractor.extractFromMostroMessage.
🤖 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/widgets/notification_item.dart`:
- Around line 101-103: The two actions are currently grouped; split
mostro_action.Action.sendDm and mostro_action.Action.chatMessage into separate
cases in notification_item.dart so chatMessage continues to navigate to
context.push('/trade_detail/${notification.orderId}') while sendDm checks
notification.disputeId and, if present, navigates to
context.push('/dispute_details/${notification.disputeId}'); if disputeId is
null/absent, fall back to the previous behavior (e.g., push the trade detail or
DM tray as appropriate) and ensure notification.disputeId is persisted when
creating sendDm notifications.
---
Nitpick comments:
In
`@test/features/notifications/services/background_notification_dm_detection_test.dart`:
- Around line 254-309: Add a test that constructs a MostroMessage whose payload
is encrypted for the test session's sharedKey.public and then runs it through
the background pipeline to ensure the p2pUnwrap(session.sharedKey!) branch is
exercised: create or reuse a test Session with a matching sharedKey.public,
build an encrypted MostroMessage that would decrypt to Action.chatMessage, call
the background pipeline entrypoint
(BackgroundNotificationService.processIncoming / handleMostroMessage or
equivalent used in your codebase) and assert the emitted/produced
NotificationData has action == Action.chatMessage; this ensures the new
shared-key decrypt path (p2pUnwrap) is exercised rather than bypassed by
directly calling NotificationDataExtractor.extractFromMostroMessage.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d59d424b-cb7d-4f8c-b995-65bf6064da3c
📒 Files selected for processing (7)
lib/data/models/enums/action.dartlib/data/models/notification.dartlib/features/notifications/services/background_notification_service.dartlib/features/notifications/utils/notification_data_extractor.dartlib/features/notifications/utils/notification_message_mapper.dartlib/features/notifications/widgets/notification_item.darttest/features/notifications/services/background_notification_dm_detection_test.dart
✅ Files skipped from review due to trivial changes (2)
- lib/data/models/enums/action.dart
- lib/features/notifications/utils/notification_data_extractor.dart
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
When my order went to Active status and i stared to receive DMs from my counterpert, I wasn't getting notifications for incoming messages until I closed and reopened the app.
Then I started getting notifications for messages from the other person, but I was also getting a notification for every message I sent.
Then I closed the app again, and when I reopened it, I didn't get any message notifications from either the other person or me.
I closed and reopened it again, and I started getting notifications for incoming messages again, but also for all the messages I sent.
…s and when app is in foreground
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/features/notifications/services/background_notification_service.dart (1)
261-291: Consider adding try-catch for consistent error handling.Both
_processAdminDmand_handleP2PChatEventwrap their logic in local try-catch blocks with handler-specific error messages (e.g.,'Admin DM decrypt error','P2P chat decrypt error'). This handler relies on the parent catch, which logs a generic'Decrypt error'message, making it harder to distinguish trade-key failures in logs.♻️ Optional: Add local try-catch for consistent observability
/// Handle events matched by tradeKey (Mostro protocol + admin/dispute DMs) Future<MostroMessage?> _handleTradeKeyEvent(NostrEvent event, Session session) async { + try { final decryptedEvent = await event.unWrap(session.tradeKey.private); if (decryptedEvent.content == null) { return null; } final result = jsonDecode(decryptedEvent.content!); if (result is! List || result.isEmpty) { return null; } // Detect admin/dispute DM format that arrived via tradeKey final firstItem = result[0]; if (NostrUtils.isDmPayload(firstItem)) { if (session.orderId == null) { logger.w('DM received but session has no orderId (recipient: ${event.recipient}), skipping notification'); return null; } return MostroMessage( action: mostro_action.Action.sendDm, id: session.orderId, timestamp: event.createdAt?.millisecondsSinceEpoch, ); } final mostroMessage = MostroMessage.fromJson(result[0]); mostroMessage.timestamp = event.createdAt?.millisecondsSinceEpoch; return mostroMessage; + } catch (e) { + logger.e('Trade key decrypt error: $e'); + return null; + } }🤖 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 261 - 291, The _handleTradeKeyEvent function lacks a local try-catch around the decryption/parsing steps (event.unWrap and jsonDecode) so trade-key failures get logged only by the parent generic handler; wrap the decryption and JSON parsing in a local try-catch, log a clear, specific message like "Trade-key decrypt error" including the error and stack, and return null (or otherwise short-circuit) on error—follow the pattern used in _processAdminDm and _handleP2PChatEvent to improve observability for MostroMessage creation and handling.
🤖 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 293-311: The P2P unwrap in _handleP2PChatEvent assumes
session.sharedKey is non-null; add a defensive guard that returns null if
session.sharedKey (or its public value if applicable) is null before calling
event.p2pUnwrap(session.sharedKey!) to avoid a potential crash, and similarly
ensure the earlier matching logic that compares s?.sharedKey?.public to
event.recipient never treats two nulls as a match by requiring a non-null
recipient and non-null sharedKey.public when matching.
---
Nitpick comments:
In `@lib/features/notifications/services/background_notification_service.dart`:
- Around line 261-291: The _handleTradeKeyEvent function lacks a local try-catch
around the decryption/parsing steps (event.unWrap and jsonDecode) so trade-key
failures get logged only by the parent generic handler; wrap the decryption and
JSON parsing in a local try-catch, log a clear, specific message like "Trade-key
decrypt error" including the error and stack, and return null (or otherwise
short-circuit) on error—follow the pattern used in _processAdminDm and
_handleP2PChatEvent to improve observability for MostroMessage creation and
handling.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2424221a-52a8-43f8-821f-21806568cfd7
📒 Files selected for processing (2)
lib/background/background.dartlib/features/notifications/services/background_notification_service.dart
lib/features/notifications/services/background_notification_service.dart
Show resolved
Hide resolved
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
Re-reviewed after the latest updates. The foreground suppression, null-safe session matching, own-message filtering, and dynamic background chat subscription flow all look good. The action/type mapping remains consistent (chatMessage vs sendDm). LGTM.
2 participants
Summary
_handleTradeKeyEvent()for clarity_decryptAndProcessEvent()so P2P chat messages arriving in background are detected_handleP2PChatEvent()that decrypts viap2pUnwrap(sharedKey)and triggers a generic "New message" notificationHow it works
Previously,
_decryptAndProcessEvent()only matched events bytradeKey.public, so P2P chat messages (which arrive atsharedKey.public) were silently dropped. Now, when no tradeKey match is found, sessions are checked bysharedKey.public. Matched events are decrypted withp2pUnwrapand produce the same generic chat notification used for admin DMs.Test plan
flutter analyze— zero issues expectedSummary by CodeRabbit
New Features
Improvements
Tests