Skip to content

Conversation

@Catrya
Copy link
Member

@Catrya Catrya commented Sep 2, 2025

fix #269

  • Add handling for 10 specific cant-do reasons in snackbar notifications
  • Implement custom messages for: pending_order_exists, not_allowed_by_status, invalid_invoice, invalid_trade_index, is_not_your_order, invalid_signature, invalid_peer, invalid_pubkey, order_already_canceled, out_of_range_sats_amount
  • With other cant-do it will continue to be seen as before: "Action not allowed"

Summary by CodeRabbit

  • New Features

    • Notifications now show specific localized reasons when actions can’t proceed (e.g., pending order, invalid index, out-of-range amount), improving clarity.
  • Style

    • Shortened, more generic error messages in English, Spanish, and Italian for consistent, concise user-facing text.
  • Tests

    • Added mock support for sending notifications to improve test coverage of notification handling.

  - Add handling for 10 specific cant-do reasons in snackbar notifications
  - Implement custom messages for: pending_order_exists, not_allowed_by_status, invalid_invoice, invalid_trade_index, is_not_your_order, invalid_signature, invalid_peer, invalid_pubkey, order_already_canceled, out_of_range_sats_amount
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 2, 2025

Walkthrough

The PR adds reason-specific cantDo notification handling, shortens several localization strings (EN/ES/IT), updates two localization method signatures used in the trade detail widget, and adds a sendNotification stub to test mocks.

Changes

Cohort / File(s) Summary
Notification handling
lib/shared/widgets/notification_listener_widget.dart
Adds explicit mapping of cantDo reasons to localized strings (pending_order_exists, not_allowed_by_status, invalid_invoice, invalid_trade_index, is_not_your_order, invalid_signature, invalid_peer, invalid_pubkey, order_already_canceled, out_of_range_sats_amount); imports Action enum; falls back to generic title for unknown reasons.
Trade detail widget messaging
lib/features/trades/widgets/mostro_message_detail_widget.dart
Removes arguments from two localization lookups: notAllowedByStatus() and outOfRangeSatsAmount() now called without parameters.
Localization updates (EN/ES/IT)
lib/l10n/intl_en.arb, lib/l10n/intl_es.arb, lib/l10n/intl_it.arb
Replace four messages (invalidTradeIndex, notAllowedByStatus, outOfRangeSatsAmount, pendingOrderExists) with shorter, generic text and remove placeholders.
Test mocks
test/mocks.mocks.dart
Adds MockOrderNotifier.sendNotification(_i27.Action? action, {Map<String, dynamic>? values, bool? isTemporary = false, String? eventId}) stub via noSuchMethod.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant App
  participant NotificationListener
  participant I18n as S
  participant UI as Snackbar

  User->>App: Trigger action/event
  App-->>NotificationListener: Emit notification { action: cantDo, values: { reason, action } }
  NotificationListener->>NotificationListener: Inspect values['action']== 'cantDo' and values['reason']
  alt Recognized reason (e.g., pending_order_exists, out_of_range_sats_amount, not_allowed_by_status)
    NotificationListener->>I18n: Request localized message for that reason
  else Unknown reason
    NotificationListener->>I18n: Request generic mapped title
  end
  NotificationListener->>UI: Show Snackbar with localized message
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Assessment against linked issues

Objective Addressed Explanation
Improve cant-do messages to show specific reasons [#269]
Use protocol-suggested messages for reasons [#269] Message text was changed, but exact protocol wording match is not verified in this diff.

Assessment against linked issues: Out-of-scope changes

Code Change Explanation
Localization text and signature changes (lib/l10n/intl_en.arb, lib/l10n/intl_es.arb, lib/l10n/intl_it.arb) These modify user-facing strings and remove placeholders / parameters; not required by #269 which asks only to surface reason field — changing message content/params may be beyond mapping behavior.
Localization method signature updates (generated S methods) (lib/features/trades/widgets/mostro_message_detail_widget.dart references changed; corresponding generated methods updated) Changing generated localization signatures affects call sites broadly; not explicitly required by #269 and may have cascading impact.
Mock addition (test/mocks.mocks.dart) Test stub addition modifies test harness rather than cantDo behavior; not directly required by #269.

Possibly related PRs

Suggested reviewers

  • chebizarro
  • grunch

Poem

I thump my paw and read the log,
Cant-do reasons hop out of the fog.
From sats too large to status amiss,
Snackbars now tell what the problem is.
Carrots, tests, and strings aligned—hip hip, we ship! 🥕🐇


📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 37f963b and ef8487c.

📒 Files selected for processing (1)
  • lib/shared/widgets/notification_listener_widget.dart (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • lib/shared/widgets/notification_listener_widget.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
✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch cant-do-reasons

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
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore or @coderabbit ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

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: 2

Caution

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

⚠️ Outside diff range comments (2)
lib/features/trades/widgets/mostro_message_detail_widget.dart (2)

241-269: Make cantDo fallback user-friendly and aligned with PR

The default currently leaks internal state/action text. Per PR goals, fall back to a generic “Action not allowed” message.

-      default:
-        return '${tradeState.status.toString()} - ${tradeState.action}'; // This is a fallback message for developers
+      default:
+        // Generic fallback for unmapped cant-do reasons
+        return S.of(context)!.notificationCantDoMessage;

241-269: Pass real min/max to outOfRangeFiatAmount (avoid literal placeholders in UI)

Currently it renders "{fiat_min}" / "{fiat_max}" literally. Use configured bounds when available; otherwise fall back to the generic cant-do message.

-      case CantDoReason.outOfRangeFiatAmount:
-        return S.of(context)!.outOfRangeFiatAmount('{fiat_min}', '{fiat_max}');
+      case CantDoReason.outOfRangeFiatAmount:
+        final mi = ref.read(orderRepositoryProvider).mostroInstance;
+        final min = mi?.minOrderAmount?.toString();
+        final max = mi?.maxOrderAmount?.toString();
+        if (min != null && max != null) {
+          return S.of(context)!.outOfRangeFiatAmount(min, max);
+        }
+        return S.of(context)!.notificationCantDoMessage;
🧹 Nitpick comments (5)
lib/l10n/intl_es.arb (1)

139-141: Minor wording consistency: “sats” lowercase.

Most strings elsewhere use “sats”. Consider “La cantidad de sats…”.

lib/l10n/intl_en.arb (1)

140-140: Style nit: use “sats” (lowercase) for consistency.

“The sats amount is out of the allowed range.”

lib/shared/widgets/notification_listener_widget.dart (2)

41-60: Replace long if-else chain with a map for clarity and maintainability.

This reduces branching and makes adding new reasons trivial.

-            if (cantDoReason == 'pending_order_exists') {
-              message = S.of(context)!.pendingOrderExists;
-            } else if (cantDoReason == 'not_allowed_by_status') {
-              message = S.of(context)!.notAllowedByStatus;
-            } else if (cantDoReason == 'invalid_invoice') {
-              message = S.of(context)!.invalidInvoice;
-            } else if (cantDoReason == 'invalid_trade_index') {
-              message = S.of(context)!.invalidTradeIndex;
-            } else if (cantDoReason == 'is_not_your_order') {
-              message = S.of(context)!.isNotYourOrder;
-            } else if (cantDoReason == 'invalid_signature') {
-              message = S.of(context)!.invalidSignature;
-            } else if (cantDoReason == 'invalid_peer') {
-              message = S.of(context)!.invalidPeer;
-            } else if (cantDoReason == 'invalid_pubkey') {
-              message = S.of(context)!.invalidPubkey;
-            } else if (cantDoReason == 'order_already_canceled') {
-              message = S.of(context)!.orderAlreadyCanceled;
-            } else if (cantDoReason == 'out_of_range_sats_amount') {
-              message = S.of(context)!.outOfRangeSatsAmount;
-            } else {
-              // Use generic cant-do message for other reasons
-              message = NotificationMessageMapper.getLocalizedTitle(context, next.action!);
-            }
+            final reasons = <String, String>{
+              'pending_order_exists': S.of(context)!.pendingOrderExists,
+              'not_allowed_by_status': S.of(context)!.notAllowedByStatus,
+              'invalid_invoice': S.of(context)!.invalidInvoice,
+              'invalid_trade_index': S.of(context)!.invalidTradeIndex,
+              'is_not_your_order': S.of(context)!.isNotYourOrder,
+              'invalid_signature': S.of(context)!.invalidSignature,
+              'invalid_peer': S.of(context)!.invalidPeer,
+              'invalid_pubkey': S.of(context)!.invalidPubkey,
+              'order_already_canceled': S.of(context)!.orderAlreadyCanceled,
+              'out_of_range_sats_amount': S.of(context)!.outOfRangeSatsAmount,
+            };
+            message = (cantDoReason != null && reasons.containsKey(cantDoReason))
+                ? reasons[cantDoReason]!
+                : NotificationMessageMapper.getLocalizedTitle(context, next.action!);

62-67: Optional: handle additional likely reason ‘out_of_range_fiat_amount’.

If protocol may send it, map to S.of(context)!.outOfRangeFiatAmount to keep parity.

Would you like me to add this mapping now?

lib/features/trades/widgets/mostro_message_detail_widget.dart (1)

241-269: Deduplicate cantDo reason-to-message mapping

Cant-do reason mapping is now in multiple places (this widget and notification listener). Extract a single mapper (e.g., CantDoReasonLocalizer.map(reason, context, values)) and reuse to avoid drift.

I can sketch a small helper and update both call sites if you want.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 6f710e8 and 37f963b.

📒 Files selected for processing (6)
  • lib/features/trades/widgets/mostro_message_detail_widget.dart (1 hunks)
  • lib/l10n/intl_en.arb (2 hunks)
  • lib/l10n/intl_es.arb (2 hunks)
  • lib/l10n/intl_it.arb (2 hunks)
  • lib/shared/widgets/notification_listener_widget.dart (2 hunks)
  • test/mocks.mocks.dart (1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
lib/l10n/intl_{en,es,it}.arb

📄 CodeRabbit inference engine (CLAUDE.md)

Add new localization keys to all three ARB files (en, es, it)

Files:

  • lib/l10n/intl_es.arb
  • lib/l10n/intl_en.arb
  • lib/l10n/intl_it.arb
{lib/*.dart,lib/!(generated)/**/*.dart}

📄 CodeRabbit inference engine (CLAUDE.md)

{lib/*.dart,lib/!(generated)/**/*.dart}: Use localized strings; replace hardcoded user-facing text with S.of(context).keyName
Prefer const constructors and const where possible
Use latest Flutter/Dart APIs (e.g., withValues() instead of deprecated withOpacity())

Files:

  • lib/shared/widgets/notification_listener_widget.dart
  • lib/features/trades/widgets/mostro_message_detail_widget.dart
{lib/*.dart,lib/!(generated)/**/*.dart,test/**/*.dart}

📄 CodeRabbit inference engine (CLAUDE.md)

{lib/*.dart,lib/!(generated)/**/*.dart,test/**/*.dart}: Check mounted before using BuildContext after await (async gaps)
Remove unused imports and dependencies

Files:

  • lib/shared/widgets/notification_listener_widget.dart
  • lib/features/trades/widgets/mostro_message_detail_widget.dart
  • test/mocks.mocks.dart
**/*.dart

📄 CodeRabbit inference engine (CLAUDE.md)

All code comments, variable names, and function names must be in English

Files:

  • lib/shared/widgets/notification_listener_widget.dart
  • lib/features/trades/widgets/mostro_message_detail_widget.dart
  • test/mocks.mocks.dart
**/*.mocks.dart

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.mocks.dart: Never manually edit Mockito-generated *.mocks.dart files
Do not add ignore comments to *.mocks.dart; regenerate instead if analyzer issues appear

Files:

  • test/mocks.mocks.dart
test/mocks.mocks.dart

📄 CodeRabbit inference engine (CLAUDE.md)

test/mocks.mocks.dart: Do not manually modify test/mocks.mocks.dart (auto-generated by Mockito)
Do not add additional ignore directives; file already has file-level ignores

Files:

  • test/mocks.mocks.dart
test/**

📄 CodeRabbit inference engine (CLAUDE.md)

Place unit tests under test/

Files:

  • test/mocks.mocks.dart
🧠 Learnings (5)
📚 Learning: 2025-08-28T20:32:34.625Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-28T20:32:34.625Z
Learning: Applies to lib/l10n/intl_{en,es,it}.arb : Add new localization keys to all three ARB files (en, es, it)

Applied to files:

  • lib/l10n/intl_es.arb
  • lib/l10n/intl_it.arb
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, Riverpod code generation is used with `Riverpod` annotations. Providers like `eventStorageProvider` are generated in `.g.dart` files from annotated functions in the main provider files. These providers are accessible by importing the main provider file (e.g., `mostro_service_provider.dart`), not by importing a separate provider file.

Applied to files:

  • lib/shared/widgets/notification_listener_widget.dart
📚 Learning: 2025-08-28T20:32:34.625Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-28T20:32:34.625Z
Learning: Use Riverpod for state management with Notifiers encapsulating business logic; access data only through repositories

Applied to files:

  • lib/shared/widgets/notification_listener_widget.dart
📚 Learning: 2025-08-15T01:37:12.243Z
Learnt from: Catrya
PR: MostroP2P/mobile#270
File: lib/shared/widgets/order_filter.dart:133-135
Timestamp: 2025-08-15T01:37:12.243Z
Learning: The MostroP2P/mobile project requires Flutter >=3.27.0 as specified in pubspec.yaml, which supports the Color.withValues() method, so usage of withValues() throughout the codebase is valid and should not be flagged as a compatibility issue.

Applied to files:

  • lib/shared/widgets/notification_listener_widget.dart
📚 Learning: 2025-08-28T20:32:34.625Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-28T20:32:34.625Z
Learning: Applies to test/mocks.mocks.dart : Do not manually modify `test/mocks.mocks.dart` (auto-generated by Mockito)

Applied to files:

  • test/mocks.mocks.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 (5)
lib/l10n/intl_es.arb (1)

124-147: Generic messages parity and usage verified
intl_it.arb includes the same four no-argument keys, and no call sites invoke them with placeholders.

lib/l10n/intl_en.arb (1)

124-147: Generic no-arg messages verified, Italian locale in sync. Verified there are no remaining calls passing arguments to notAllowedByStatus or outOfRangeSatsAmount, and intl_it.arb contains matching no-arg entries.

lib/shared/widgets/notification_listener_widget.dart (1)

4-4: Unify Action import path: notification_listener_widget.dart imports from package:mostro_mobile/data/models/enums/action.dart while mocks/tests import via package:mostro_mobile/data/enums.dart. Confirm that data/enums.dart re-exports the same Action enum; otherwise, consolidate to one import to prevent duplicate types.

lib/features/trades/widgets/mostro_message_detail_widget.dart (1)

251-256: Approve zero-arg localized getters Verified via ripgrep that there are no remaining calls to S.of(context)!.notAllowedByStatus( or S.of(context)!.outOfRangeSatsAmount(; changes approved.

lib/l10n/intl_it.arb (1)

124-124: All four translation keys are present in intl_en.arb, intl_es.arb, and intl_it.arb—please regenerate l10n
Run your locale generation command (e.g. flutter pub run intl_utils:generate).

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.

great job! utACK

@grunch grunch merged commit 1f1d48d into main Sep 3, 2025
2 checks passed
@grunch grunch deleted the cant-do-reasons branch September 3, 2025 14:13
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.

Improve cant-do messages to show specific reasons

3 participants