Skip to content

Conversation

@Catrya
Copy link
Member

@Catrya Catrya commented Oct 28, 2025

  - Add Lightning address usage detection in HoldInvoicePaymentAccepted
  - Search previous order confirmation messages for buyerInvoice
  - Display lightningAddressUsed notification for buy order makers
  - Use existing notification system for consistent UX
  - Only applies to buyers who used their configured Lightning address

Summary by CodeRabbit

  • New Features

    • Added notification detection for Lightning address usage during order payment flows for buyers.
  • Improvements

    • Enhanced error handling for notification detection with improved logging to ensure main flow stability.

      - Add Lightning address usage detection in HoldInvoicePaymentAccepted
      - Search previous order confirmation messages for buyerInvoice
      - Display lightningAddressUsed notification for buy order makers
      - Use existing notification system for consistent UX
      - Only applies to buyers who used their configured Lightning address
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 28, 2025

Walkthrough

Adds runtime notification logic to the order notifier to detect Lightning address usage during the holdInvoicePaymentAccepted flow. For buyer users, the implementation reads stored messages, identifies an incoming NewOrder message with a valid buyerInvoice, validates the Lightning address format, and triggers a lightningAddressUsed notification with error handling.

Changes

Cohort / File(s) Summary
Lightning Address Detection Notification
lib/features/order/notfiers/abstract_mostro_notifier.dart
Adds runtime detection of Lightning address usage during holdInvoicePaymentAccepted flow; reads and validates stored messages for buyers, triggers lightningAddressUsed notification, includes error handling

Sequence Diagram(s)

sequenceDiagram
    participant Flow as holdInvoicePaymentAccepted Flow
    participant Notifier as AbstractMostroNotifier
    participant Store as Message Store
    participant Validator as Lightning Address Validator
    participant Notify as Notification System
    
    Flow->>Notifier: holdInvoicePaymentAccepted triggered
    Notifier->>Notifier: Check if current user is buyer
    alt Is Buyer
        Notifier->>Store: Read stored messages for order
        Store-->>Notifier: Messages retrieved
        Notifier->>Notifier: Find NewOrder message with buyerInvoice
        alt Found & Valid
            Notifier->>Validator: Validate Lightning address format
            alt Valid Format
                Validator-->>Notifier: Address is valid
                Notifier->>Notify: Trigger lightningAddressUsed notification
                Notify-->>Notifier: Notification sent
            else Invalid Format
                Validator-->>Notifier: Address validation failed
                Notifier->>Notifier: Log error (non-blocking)
            end
        else Not Found
            Notifier->>Notifier: Log error (non-blocking)
        end
    else Not Buyer
        Notifier->>Notifier: Skip Lightning detection
    end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Focused single-file modification with straightforward validation logic
  • Requires understanding of the notification flow and message storage mechanism
  • Pay attention to Lightning address validation logic accuracy and error handling paths to ensure non-blocking behavior

Possibly related PRs

Suggested reviewers

  • grunch

Poem

🐰 A lightning bolt in the buyer's hand,
We notify when addresses land,
Validation swift, no errors block,
Just smooth notification flow that won't shock! ⚡

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "feat: show notification when configured Lightning address is used" accurately captures the main change in the pull request. The changeset adds runtime notification logic to detect when a buyer uses a Lightning address during the holdInvoicePaymentAccepted flow and triggers a lightningAddressUsed notification. The title is concise, clear, and specific enough for a teammate to understand that a new notification feature is being added for Lightning address usage, which aligns directly with the primary functionality being implemented.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch lnaddress-notific

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

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
lib/features/order/notfiers/abstract_mostro_notifier.dart (1)

272-278: Consider optimizing message retrieval and adding explanatory comment.

The current implementation loads all messages for the order to find a single Action.newOrder message with a buyerInvoice. Consider these improvements:

  1. Performance: If the storage API supports it, add a more targeted query method to retrieve only Action.newOrder messages, avoiding the need to load all messages.

  2. Documentation: Add a brief comment explaining that the buyerInvoice field in the order confirmation message contains the Lightning address when one was used.

Example comment addition:

           try {
             final storage = ref.read(mostroStorageProvider);
             final messages = await storage.getAllMessagesForOrderId(orderId);
             
-            // Find the order confirmation message (incoming Action.newOrder)
+            // Find the order confirmation message (incoming Action.newOrder)
+            // The buyerInvoice field contains the Lightning address if one was used
             final orderConfirmation = messages
                 .where((m) => m.action == Action.newOrder)
                 .where((m) => m.getPayload<Order>()?.buyerInvoice != null)
                 .firstOrNull;
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 767101e and 37cc20e.

📒 Files selected for processing (1)
  • lib/features/order/notfiers/abstract_mostro_notifier.dart (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
lib/features/**

📄 CodeRabbit inference engine (AGENTS.md)

Place application feature code under lib/features//, grouped by domain

Files:

  • lib/features/order/notfiers/abstract_mostro_notifier.dart
**/*.dart

📄 CodeRabbit inference engine (AGENTS.md)

**/*.dart: Use Dart formatter defaults (two-space indentation, trailing commas) and run flutter format before committing
Resolve all Flutter analyzer warnings (flutter analyze must be clean)

**/*.dart: Remove unused imports and unused dependencies
Do not add // ignore: must_be_immutable to classes within generated files; rely on regeneration and existing file-level ignores

Files:

  • lib/features/order/notfiers/abstract_mostro_notifier.dart
lib/**/*.dart

📄 CodeRabbit inference engine (AGENTS.md)

Name Riverpod providers as Provider or Notifier

lib/**/*.dart: Always use localized strings via S.of(context)!.keyName; avoid hardcoded user-facing text
Use clear, concise English for variable names, function names, and code comments; all comments must be in English
Target zero flutter analyze issues
Use latest Flutter/Dart APIs (e.g., Color.withValues() instead of withOpacity())
After async operations, check mounted before using BuildContext
Use const constructors where possible

Files:

  • lib/features/order/notfiers/abstract_mostro_notifier.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build
🔇 Additional comments (1)
lib/features/order/notfiers/abstract_mostro_notifier.dart (1)

267-294: Add mounted check after async operation.

After the async call at line 272 (await storage.getAllMessagesForOrderId(orderId)), you should verify that the notifier is still mounted before showing the notification at line 287. Similar mounted checks exist elsewhere in this file (lines 596-597, 606-607).

Apply this diff to add the mounted check:

           if (orderConfirmation != null) {
             final confirmationOrder = orderConfirmation.getPayload<Order>();
             final buyerInvoice = confirmationOrder?.buyerInvoice;
             
             if (buyerInvoice != null && _isValidLightningAddress(buyerInvoice)) {
+              // Check if still mounted after async operation
+              if (!mounted) return;
+              
               // Show Lightning address used notification
               final notificationNotifier = ref.read(notificationActionsProvider.notifier);
               notificationNotifier.showCustomMessage('lightningAddressUsed');
             }
           }

As per coding guidelines.

⛔ Skipped due to learnings
Learnt from: Catrya
PR: MostroP2P/mobile#327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:157-182
Timestamp: 2025-10-21T21:47:03.451Z
Learning: In MostroP2P/mobile, for Action.canceled handling in abstract_mostro_notifier.dart (Riverpod StateNotifier), do not add mounted checks after async sessionNotifier.deleteSession(orderId) as they break order state synchronization during app restart. The Action.canceled flow contains critical business logic that must complete fully; Riverpod handles provider disposal automatically. Mounted checks should only protect UI operations, not business logic in StateNotifiers.

@Catrya Catrya requested a review from grunch October 28, 2025 16:19
Copy link
Member

@grunch grunch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK

@grunch grunch merged commit 4821c0f into main Nov 10, 2025
2 checks passed
@grunch grunch deleted the lnaddress-notific branch November 10, 2025 19:30
AndreaDiazCorreia pushed a commit that referenced this pull request Nov 15, 2025
- Add Lightning address usage detection in HoldInvoicePaymentAccepted
      - Search previous order confirmation messages for buyerInvoice
      - Display lightningAddressUsed notification for buy order makers
      - Use existing notification system for consistent UX
      - Only applies to buyers who used their configured Lightning address
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants