Refactor/chat provider #714
Conversation
WalkthroughAdds MessageSenderService and NostrTagBuilderService to centralize building and sending messages, reactions, replies, and deletions; introduces ReactionComparisonService for reaction-diff logic; updates chat provider to use these services; removes local tag/reaction helpers; adjusts optimistic message creation signature; and adds unit tests and test helpers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Chat as ChatProvider
participant Sender as MessageSenderService
participant Builder as NostrTagBuilderService
participant API as sendMessageToGroup
rect #E8F6FF
User->>Chat: send message/reply/reaction/deletion
Chat->>Sender: sendMessage/sendReply/sendReaction/sendDeletion(...)
end
alt send message
Sender->>API: send(pubkey, groupId, message: content, kind: 9, tags?)
else send reply
Sender->>Builder: buildReplyTags(replyToMessageId)
Builder-->>Sender: tags
Sender->>API: send(..., kind: 9, tags)
else send reaction
Sender->>Builder: buildReactionTags(messageId, messagePubkey, messageKind)
Builder-->>Sender: tags
Sender->>API: send(..., kind: 7, tags)
else send deletion
Sender->>Builder: buildDeletionTags(messageId, messagePubkey, messageKind)
Builder-->>Sender: tags
Sender->>API: send(..., kind: 5, tags, message: "")
end
API-->>Sender: MessageWithTokens
Sender-->>Chat: MessageWithTokens
Chat-->>User: result
sequenceDiagram
autonumber
participant Chat as ChatProvider
participant RCS as ReactionComparisonService
Chat->>RCS: areDifferent(oldReactions, newReactions)
RCS-->>Chat: bool
alt true
Chat->>Chat: update state / trigger UI refresh
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.dart📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
Files:
🧠 Learnings (1)📚 Learning: 2025-08-08T13:39:00.500ZApplied to files:
⏰ 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)
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/domain/services/message_sending_service_test.dart (1)
5-36: Consolidate the shared Tag test doubles
MockTagandmockTagFromVecduplicate the definitions intest/domain/services/nostr_tag_builder_test.dart. Please pull these helpers into a shared test fixture (e.g.,test/support/tag_test_utils.dart) and import them from both suites to avoid divergence.test/domain/services/reaction_comparison_service_test.dart (1)
27-161: Add coverage for duplicate reactionsPlease add a case where one list contains the same user reacting twice with the same emoji while the other list does not. That regression test will lock in the upcoming fix to
ReactionComparisonService._areUserListsEqual, ensuring we surface duplicated reactions as a difference.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/config/providers/chat_provider.dart(6 hunks)lib/domain/services/message_sending_service.dart(1 hunks)lib/domain/services/nostr_tag_builder.dart(1 hunks)lib/domain/services/reaction_comparison_service.dart(1 hunks)test/domain/services/message_sending_service_test.dart(1 hunks)test/domain/services/nostr_tag_builder_test.dart(1 hunks)test/domain/services/reaction_comparison_service_test.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.dart
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
**/*.dart: Always declare the type of each variable and function (parameters and return value). Avoid using 'any'. Create necessary types.
Don't leave blank lines within a function.
One export per file.
Use PascalCase for classes.
Use camelCase for variables, functions, and methods.
Use underscores_case for file and directory names.
Use UPPERCASE for environment variables. Avoid magic numbers and define constants.
Start each function with a verb.
Use verbs for boolean variables. Example: isLoading, hasError, canDelete, etc.
Use complete words instead of abbreviations and correct spelling, except for standard and well-known abbreviations (API, URL, i, j, err, ctx, req, res, next).
Write short functions with a single purpose. Less than 20 instructions.
Name functions with a verb and something else. If it returns a boolean, use isX or hasX, canX, etc. If it doesn't return anything, use executeX or saveX, etc.
Avoid nesting blocks by early checks and returns, or extraction to utility functions.
Use higher-order functions (map, filter, reduce, etc.) to avoid function nesting. Use arrow functions for simple functions (less than 3 instructions). Use named functions for non-simple functions.
Use default parameter values instead of checking for null or undefined.
Reduce function parameters using RO-RO: use an object to pass multiple parameters and to return results. Declare necessary types for input arguments and output.
Use a single level of abstraction in functions.
Don't abuse primitive types and encapsulate data in composite types.
Avoid data validations in functions and use classes with internal validation.
Prefer immutability for data. Use readonly for data that doesn't change. Use 'as const' for literals that don't change.
Declare interfaces to define contracts.
Write small classes with a single purpose. Less than 200 instructions, less than 10 public methods, less than 10 properties.
Use exceptions to handle errors you don't expect. If you catch an exception, it sh...
Files:
test/domain/services/message_sending_service_test.dartlib/config/providers/chat_provider.dartlib/domain/services/nostr_tag_builder.darttest/domain/services/reaction_comparison_service_test.dartlib/domain/services/reaction_comparison_service.darttest/domain/services/nostr_tag_builder_test.dartlib/domain/services/message_sending_service.dart
**/*_test.dart
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
**/*_test.dart: Follow the Arrange-Act-Assert convention for tests.
Name test variables clearly. Follow the convention: inputX, mockX, actualX, expectedX, etc.
Write unit tests for each public function. Use test doubles to simulate dependencies, except for third-party dependencies that are not expensive to execute.
Write acceptance tests for each module. Follow the Given-When-Then convention.
Use the standard widget testing for Flutter.
Files:
test/domain/services/message_sending_service_test.darttest/domain/services/reaction_comparison_service_test.darttest/domain/services/nostr_tag_builder_test.dart
🔇 Additional comments (4)
lib/domain/services/nostr_tag_builder.dart (2)
4-41: LGTM: Clean service extraction.The class is well-structured with a single responsibility (building Nostr tags), follows Dart naming conventions, properly declares all types, and uses dependency injection for testability.
16-20: Unify ‘p’ tag vector format per Nostr spec
The empty-string relay parameter inbuildReactionTagsdiffers frombuildDeletionTags. Confirm whether the relay field is optional per NIP-10; then either add''to the deletion tags or remove it from the reaction tags for consistency.lib/domain/services/message_sending_service.dart (2)
4-105: Clean refactor with good separation of concerns.The service properly delegates tag building to
NostrTagBuilderand encapsulates message dispatch logic. Dependency injection is well-implemented for testability, and all methods follow Dart conventions with clear verb-based names and required parameters.
25-26: No action required: sendMessageToGroup is explicitly imported
message_sending_service.dart imports package:whitenoise/src/rust/api/messages.dart, which defines sendMessageToGroup.
josefinalliende
force-pushed
the
refactor/chat-provider
branch
from
8d0baf5 to
395c3d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
test/domain/services/reaction_comparison_service_test.dart (1)
143-181: Add test for per-emoji duplicate count changes (same user sets, different multiplicities).Current tests miss the case where total counts are equal and user sets match per emoji, but duplicate counts shift between emojis. This will expose a bug in the service that ignores multiplicities.
@@ group('areDifferent', () { + test('returns true when per-emoji duplicate counts differ with same user sets', () { + // A has two 👍 from alice and one ❤️ from bob + final reactionsA = [ + Reaction(emoji: '👍', user: alice), + Reaction(emoji: '👍', user: alice), + Reaction(emoji: '❤️', user: bob), + ]; + // B has one 👍 from alice and two ❤️ from bob + final reactionsB = [ + Reaction(emoji: '👍', user: alice), + Reaction(emoji: '❤️', user: bob), + Reaction(emoji: '❤️', user: bob), + ]; + final result = ReactionComparisonService.areDifferent( + reactionsA, + reactionsB, + ); + expect(result, true); + });lib/config/providers/chat_provider.dart (1)
23-24: Inject MessageSenderService via getIt (don’t instantiate directly).Direct construction reduces testability and violates the project’s DI guideline. Resolve from getIt (or pass via provider/factory).
+import 'package:get_it/get_it.dart'; @@ - final _messageSenderService = MessageSenderService(); + final MessageSenderService _messageSenderService = GetIt.I<MessageSenderService>();Ensure MessageSenderService is registered in getIt at startup.
lib/domain/services/nostr_tag_builder_service.dart (1)
11-21: Build tags concurrently to reduce latency.Use Future.wait to parallelize tag creation; preserves order and improves responsiveness.
Future<List<Tag>> buildReactionTags({ @@ - return [ - await _tagFromVecFn(vec: ['e', messageId]), - await _tagFromVecFn(vec: ['p', messagePubkey, '']), - await _tagFromVecFn(vec: ['k', messageKind.toString()]), - ]; + return Future.wait<Tag>([ + _tagFromVecFn(vec: ['e', messageId]), + _tagFromVecFn(vec: ['p', messagePubkey, '']), + _tagFromVecFn(vec: ['k', messageKind.toString()]), + ]); } @@ - return [ - await _tagFromVecFn(vec: ['e', messageId]), - await _tagFromVecFn(vec: ['p', messagePubkey]), - await _tagFromVecFn(vec: ['k', messageKind.toString()]), - ]; + return Future.wait<Tag>([ + _tagFromVecFn(vec: ['e', messageId]), + _tagFromVecFn(vec: ['p', messagePubkey]), + _tagFromVecFn(vec: ['k', messageKind.toString()]), + ]); }Also applies to: 31-41
lib/domain/services/message_sender_service.dart (3)
47-68: Remove blank line within function.Line 60 contains a blank line, which violates the coding guideline: "Don't leave blank lines within a function."
As per coding guidelines.
Apply this diff:
}) async { final tags = await _tagBuilder.buildReactionTags( messageId: messageId, messagePubkey: messagePubkey, messageKind: messageKind, ); - return _sendMessageToGroupFn( pubkey: pubkey,
70-87: Remove blank line within function.Line 79 contains a blank line, which violates the coding guideline: "Don't leave blank lines within a function."
As per coding guidelines.
Apply this diff:
}) async { final tags = await _tagBuilder.buildReplyTags( replyToMessageId: replyToMessageId, ); - return _sendMessageToGroupFn( pubkey: pubkey,
89-109: Remove blank line within function.Line 101 contains a blank line, which violates the coding guideline: "Don't leave blank lines within a function."
As per coding guidelines.
Apply this diff:
}) async { final tags = await _tagBuilder.buildDeletionTags( messageId: messageId, messagePubkey: messagePubkey, messageKind: messageKind, ); - return _sendMessageToGroupFn( pubkey: pubkey,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/config/providers/chat_provider.dart(6 hunks)lib/domain/services/message_sender_service.dart(1 hunks)lib/domain/services/nostr_tag_builder_service.dart(1 hunks)lib/domain/services/reaction_comparison_service.dart(1 hunks)test/domain/services/message_sender_service_test.dart(1 hunks)test/domain/services/nostr_tag_builder_service_test.dart(1 hunks)test/domain/services/reaction_comparison_service_test.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.dart
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
**/*.dart: Always declare the type of each variable and function (parameters and return value). Avoid using 'any'. Create necessary types.
Don't leave blank lines within a function.
One export per file.
Use PascalCase for classes.
Use camelCase for variables, functions, and methods.
Use underscores_case for file and directory names.
Use UPPERCASE for environment variables. Avoid magic numbers and define constants.
Start each function with a verb.
Use verbs for boolean variables. Example: isLoading, hasError, canDelete, etc.
Use complete words instead of abbreviations and correct spelling, except for standard and well-known abbreviations (API, URL, i, j, err, ctx, req, res, next).
Write short functions with a single purpose. Less than 20 instructions.
Name functions with a verb and something else. If it returns a boolean, use isX or hasX, canX, etc. If it doesn't return anything, use executeX or saveX, etc.
Avoid nesting blocks by early checks and returns, or extraction to utility functions.
Use higher-order functions (map, filter, reduce, etc.) to avoid function nesting. Use arrow functions for simple functions (less than 3 instructions). Use named functions for non-simple functions.
Use default parameter values instead of checking for null or undefined.
Reduce function parameters using RO-RO: use an object to pass multiple parameters and to return results. Declare necessary types for input arguments and output.
Use a single level of abstraction in functions.
Don't abuse primitive types and encapsulate data in composite types.
Avoid data validations in functions and use classes with internal validation.
Prefer immutability for data. Use readonly for data that doesn't change. Use 'as const' for literals that don't change.
Declare interfaces to define contracts.
Write small classes with a single purpose. Less than 200 instructions, less than 10 public methods, less than 10 properties.
Use exceptions to handle errors you don't expect. If you catch an exception, it sh...
Files:
lib/domain/services/nostr_tag_builder_service.dartlib/domain/services/message_sender_service.dartlib/domain/services/reaction_comparison_service.darttest/domain/services/reaction_comparison_service_test.dartlib/config/providers/chat_provider.darttest/domain/services/nostr_tag_builder_service_test.darttest/domain/services/message_sender_service_test.dart
**/*_test.dart
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
**/*_test.dart: Follow the Arrange-Act-Assert convention for tests.
Name test variables clearly. Follow the convention: inputX, mockX, actualX, expectedX, etc.
Write unit tests for each public function. Use test doubles to simulate dependencies, except for third-party dependencies that are not expensive to execute.
Write acceptance tests for each module. Follow the Given-When-Then convention.
Use the standard widget testing for Flutter.
Files:
test/domain/services/reaction_comparison_service_test.darttest/domain/services/nostr_tag_builder_service_test.darttest/domain/services/message_sender_service_test.dart
🔇 Additional comments (9)
test/domain/services/nostr_tag_builder_service_test.dart (1)
55-69: LGTM: k tag is correctly stringified and asserted.The test cleanly validates NIP-25 k tag serialization via the injectable factory.
lib/config/providers/chat_provider.dart (5)
153-158: LGTM: sendMessage delegated to service with content param.Parameter mapping looks correct and aligns with the refactor.
286-287: Heads-up: reaction diffing relies on service fix.This uses ReactionComparisonService.areDifferent; with the current set-based comparison it can miss per-emoji count changes. After fixing the service (see comment in reaction_comparison_service.dart), this call will behave correctly.
492-499: LGTM: reaction send delegated correctly.All required identifiers (messageId, sender pubkey, kind, emoji) are passed to the service.
566-571: LGTM: reply send delegated correctly.Properly routes replyToMessageId and content to the service.
654-660: LGTM: deletion send delegated correctly.messageId, messagePubkey, and kind are forwarded as required.
test/domain/services/message_sender_service_test.dart (1)
1-655: Excellent test coverage and structure!The test suite provides comprehensive coverage of MessageSenderService functionality, including:
- All public methods (sendMessage, sendReaction, sendReply, sendDeletion)
- Happy path scenarios with correct parameter verification
- Edge cases (empty content, long content, multiple emojis, different message kinds)
- Error propagation from both send path and tag-building path
- Proper use of mocks and test doubles to isolate the service under test
The tests follow the Arrange-Act-Assert pattern and clearly verify behavior without testing implementation details.
lib/domain/services/message_sender_service.dart (2)
4-6: Magic numbers successfully extracted to constants.The constants
_messageKind,_reactionKind, and_deletionKindproperly address the previous review feedback about avoiding magic numbers.Based on past review comments.
8-110: Clean implementation with effective separation of concerns.The MessageSenderService successfully centralizes message sending logic with:
- Proper dependency injection allowing testability
- Clear delegation to NostrTagBuilderService for tag construction
- Consistent use of extracted kind constants
- Well-defined public API for each message operation
The refactoring achieves the PR objective of reducing chat provider complexity while improving testability.
josefinalliende
force-pushed
the
refactor/chat-provider
branch
from
395c3d6 to
334f1e5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/config/providers/chat_provider.dart (1)
392-392: Bug: Can't assign to parameter using ??=; causes compile errorReplace
groupId ??= ...with a null-coalescing expression; parameters are not assignable.Apply this fix:
- final gId = groupId ??= state.selectedGroupId; + final gId = groupId ?? state.selectedGroupId;- final gId = groupId ??= state.selectedGroupId; + final gId = groupId ?? state.selectedGroupId;Also applies to: 405-405
🧹 Nitpick comments (3)
lib/domain/services/nostr_tag_builder_service.dart (1)
16-20: Parallelize tag creation with Future.wait to reduce latencyBuild tags concurrently instead of awaiting each sequentially.
Apply this refactor:
- return [ - await _tagFromVecFn(vec: ['e', messageId]), - await _tagFromVecFn(vec: ['p', messagePubkey, '']), - await _tagFromVecFn(vec: ['k', messageKind.toString()]), - ]; + return Future.wait([ + _tagFromVecFn(vec: ['e', messageId]), + _tagFromVecFn(vec: ['p', messagePubkey, '']), + _tagFromVecFn(vec: ['k', messageKind.toString()]), + ]);- return [ - await _tagFromVecFn(vec: ['e', messageId]), - await _tagFromVecFn(vec: ['p', messagePubkey]), - await _tagFromVecFn(vec: ['k', messageKind.toString()]), - ]; + return Future.wait([ + _tagFromVecFn(vec: ['e', messageId]), + _tagFromVecFn(vec: ['p', messagePubkey]), + _tagFromVecFn(vec: ['k', messageKind.toString()]), + ]);Also applies to: 36-40
lib/config/providers/chat_provider.dart (2)
23-23: Inject MessageSenderService via DI instead of instantiating directlyPrefer Riverpod provider or getIt singleton for services to align with project guidelines and improve testability.
As per coding guidelines.
109-109: Avoid magic number for message kind (9)Extract Nostr kind values into shared constants and reuse (e.g., in a constants file) to prevent drift with MessageSenderService.
As per coding guidelines.
Also applies to: 545-545
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/config/providers/chat_provider.dart(6 hunks)lib/domain/services/message_sender_service.dart(1 hunks)lib/domain/services/nostr_tag_builder_service.dart(1 hunks)test/domain/services/message_sender_service_test.dart(1 hunks)test/domain/services/nostr_tag_builder_service_test.dart(1 hunks)test/shared/mocks/mock_tag_test_helpers.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/domain/services/nostr_tag_builder_service_test.dart
🧰 Additional context used
📓 Path-based instructions (2)
**/*.dart
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
**/*.dart: Always declare the type of each variable and function (parameters and return value). Avoid using 'any'. Create necessary types.
Don't leave blank lines within a function.
One export per file.
Use PascalCase for classes.
Use camelCase for variables, functions, and methods.
Use underscores_case for file and directory names.
Use UPPERCASE for environment variables. Avoid magic numbers and define constants.
Start each function with a verb.
Use verbs for boolean variables. Example: isLoading, hasError, canDelete, etc.
Use complete words instead of abbreviations and correct spelling, except for standard and well-known abbreviations (API, URL, i, j, err, ctx, req, res, next).
Write short functions with a single purpose. Less than 20 instructions.
Name functions with a verb and something else. If it returns a boolean, use isX or hasX, canX, etc. If it doesn't return anything, use executeX or saveX, etc.
Avoid nesting blocks by early checks and returns, or extraction to utility functions.
Use higher-order functions (map, filter, reduce, etc.) to avoid function nesting. Use arrow functions for simple functions (less than 3 instructions). Use named functions for non-simple functions.
Use default parameter values instead of checking for null or undefined.
Reduce function parameters using RO-RO: use an object to pass multiple parameters and to return results. Declare necessary types for input arguments and output.
Use a single level of abstraction in functions.
Don't abuse primitive types and encapsulate data in composite types.
Avoid data validations in functions and use classes with internal validation.
Prefer immutability for data. Use readonly for data that doesn't change. Use 'as const' for literals that don't change.
Declare interfaces to define contracts.
Write small classes with a single purpose. Less than 200 instructions, less than 10 public methods, less than 10 properties.
Use exceptions to handle errors you don't expect. If you catch an exception, it sh...
Files:
lib/domain/services/nostr_tag_builder_service.dartlib/config/providers/chat_provider.dartlib/domain/services/message_sender_service.darttest/domain/services/message_sender_service_test.darttest/shared/mocks/mock_tag_test_helpers.dart
**/*_test.dart
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
**/*_test.dart: Follow the Arrange-Act-Assert convention for tests.
Name test variables clearly. Follow the convention: inputX, mockX, actualX, expectedX, etc.
Write unit tests for each public function. Use test doubles to simulate dependencies, except for third-party dependencies that are not expensive to execute.
Write acceptance tests for each module. Follow the Given-When-Then convention.
Use the standard widget testing for Flutter.
Files:
test/domain/services/message_sender_service_test.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: Flutter CI
🔇 Additional comments (2)
lib/domain/services/nostr_tag_builder_service.dart (1)
18-19: Verify 'p' tag shape is intentionally different between reaction and deletionReactions use ['p', pubkey, ''] while deletions use ['p', pubkey]. Confirm this aligns with the Rust API expectations/NIP spec to avoid subtle interop issues.
Also applies to: 38-38
lib/domain/services/message_sender_service.dart (1)
4-31: Solid extraction and clean delegationConstants, DI hooks, and method boundaries look good. Tests cover these paths.
Also applies to: 32-45, 47-68, 70-87, 89-109
josefinalliende
force-pushed
the
refactor/chat-provider
branch
from
334f1e5 to
b67a5a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/config/providers/chat_provider.dart(6 hunks)lib/domain/services/message_sender_service.dart(1 hunks)test/domain/services/message_sender_service_test.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.dart
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
**/*.dart: Always declare the type of each variable and function (parameters and return value). Avoid using 'any'. Create necessary types.
Don't leave blank lines within a function.
One export per file.
Use PascalCase for classes.
Use camelCase for variables, functions, and methods.
Use underscores_case for file and directory names.
Use UPPERCASE for environment variables. Avoid magic numbers and define constants.
Start each function with a verb.
Use verbs for boolean variables. Example: isLoading, hasError, canDelete, etc.
Use complete words instead of abbreviations and correct spelling, except for standard and well-known abbreviations (API, URL, i, j, err, ctx, req, res, next).
Write short functions with a single purpose. Less than 20 instructions.
Name functions with a verb and something else. If it returns a boolean, use isX or hasX, canX, etc. If it doesn't return anything, use executeX or saveX, etc.
Avoid nesting blocks by early checks and returns, or extraction to utility functions.
Use higher-order functions (map, filter, reduce, etc.) to avoid function nesting. Use arrow functions for simple functions (less than 3 instructions). Use named functions for non-simple functions.
Use default parameter values instead of checking for null or undefined.
Reduce function parameters using RO-RO: use an object to pass multiple parameters and to return results. Declare necessary types for input arguments and output.
Use a single level of abstraction in functions.
Don't abuse primitive types and encapsulate data in composite types.
Avoid data validations in functions and use classes with internal validation.
Prefer immutability for data. Use readonly for data that doesn't change. Use 'as const' for literals that don't change.
Declare interfaces to define contracts.
Write small classes with a single purpose. Less than 200 instructions, less than 10 public methods, less than 10 properties.
Use exceptions to handle errors you don't expect. If you catch an exception, it sh...
Files:
lib/config/providers/chat_provider.dartlib/domain/services/message_sender_service.darttest/domain/services/message_sender_service_test.dart
**/*_test.dart
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
**/*_test.dart: Follow the Arrange-Act-Assert convention for tests.
Name test variables clearly. Follow the convention: inputX, mockX, actualX, expectedX, etc.
Write unit tests for each public function. Use test doubles to simulate dependencies, except for third-party dependencies that are not expensive to execute.
Write acceptance tests for each module. Follow the Given-When-Then convention.
Use the standard widget testing for Flutter.
Files:
test/domain/services/message_sender_service_test.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: Flutter CI
🔇 Additional comments (14)
lib/config/providers/chat_provider.dart (6)
14-15: LGTM!The import statements are correctly structured and follow the package structure conventions.
153-158: LGTM!The refactoring correctly delegates message sending to the service while maintaining the same functionality. The parameter rename from
messagetocontentaligns with the service API.
286-286: LGTM!The centralization of reaction comparison logic into a dedicated service improves maintainability and testability.
492-499: LGTM!The refactoring correctly delegates reaction sending to the service, eliminating manual tag construction and improving maintainability.
566-571: LGTM!The refactoring correctly delegates reply sending to the service, simplifying the provider logic and centralizing tag construction.
654-660: LGTM!The refactoring correctly delegates message deletion to the service, maintaining consistency with other message operations.
test/domain/services/message_sender_service_test.dart (3)
8-33: LGTM!The mock implementation correctly implements the
MessageWithTokensinterface with appropriate defaults and propernoSuchMethodhandling.
35-90: LGTM!The test helper correctly captures method calls and allows flexible result customization, following good test practices.
92-619: LGTM!The test suite is comprehensive and well-structured:
- Follows the Arrange-Act-Assert pattern
- Variable naming follows conventions (capturedCalls, expectedResult, etc.)
- Covers all service methods (sendMessage, sendReaction, sendReply, sendDeletion)
- Includes error propagation tests
- Tests edge cases (empty content, multiple emojis, different message kinds)
lib/domain/services/message_sender_service.dart (5)
4-6: LGTM!The magic numbers have been correctly extracted to constants with descriptive names, addressing the previous review feedback. This follows the coding guideline: "Use UPPERCASE for environment variables. Avoid magic numbers and define constants."
Based on learnings.
8-30: LGTM!The class structure supports dependency injection through optional constructor parameters while providing sensible defaults. This design facilitates both production use and testing.
32-45: LGTM!The method correctly delegates to the underlying function with the appropriate kind constant, maintaining a clean separation of concerns.
47-87: LGTM!Both
sendReactionandsendReplymethods follow a consistent pattern: build the appropriate tags via the tag builder service, then delegate to the send function with the correct kind constant. The implementation is clean and maintainable.
89-110: LGTM!The
sendDeletionmethod correctly implements deletion semantics by building the appropriate tags and sending an empty message with kind 5, consistent with the Nostr protocol.
josefinalliende
force-pushed
the
refactor/chat-provider
branch
from
b67a5a7 to
4847aeb
Compare
josefinalliende
requested review from
Quwaysim,
codeswot,
erskingardner and
untreu2
|
|
||
| bool isSameSender(int index, {String? groupId}) { | ||
| final gId = groupId ??= state.selectedGroupId; | ||
| final gId = groupId ?? state.selectedGroupId; |
There was a problem hiding this comment.
I know you didn't do this but please, could you help write gId out in full? Thanks.
|
|
||
| bool isNextSameSender(int index, {String? groupId}) { | ||
| final gId = groupId ??= state.selectedGroupId; | ||
| final gId = groupId ?? state.selectedGroupId; |
There was a problem hiding this comment.
Please, let's keep some of those important comments from the original file (chat_provider) especially the ones mentioning kinds or how to handle specific events. Maybe just copy them over. Thanks.
There was a problem hiding this comment.
Ok, I'm not a huge fan of comments in code but maybe those ones are in fact useful, I willl add them back 👍🏻
I thought that by adding the the description in the test cases of those specifications (should have, must have, etc) would be enough
There was a problem hiding this comment.
I added comments back but in the nostr tags builder service cause the more relevant comments were related to the tags.
There was a problem hiding this comment.
Related to the comments about kinds, I think the kinds are already pretty clear by the variable names...
const int _messageKind = 9;
const int _reactionKind = 7;
const int _deletionKind = 5; If I add the comments back they would say basically the same but in comment form, this are the comments related to kinds that I've found in master:
int kind = 9, // Default to text message
// Send reaction message (kind 7 for reactions in Nostr)
// Nostr kind 7 = reaction
// Nostr kind 5 = deletion
If you really think is useful I can add them, but I think its just repetitive 😅
const int _messageKind = 9; // Nostr kind 9 = text message
const int _reactionKind = 7; // Nostr kind 7 = reaction
const int _deletionKind = 5; // Nostr kind 5 = deletionThere was a problem hiding this comment.
Please let me know it adding this comments back c8df2b1 is ok or if you are missing more of them
Description
We want to send media 📸 #707 . The logic for sending messages is currently in the chat provider, which is already huge and hard to test. I’d rather not make it even bigger by adding the media logic there. My instinct is to move that logic into new classes and cover it with tests, so it’s easier to avoid breaking things later (or at least get warned by the tests, haha).
This PR
What to check:
Type of Change
Checklist
just precommitto ensure that formatting and linting are correctjust check-flutter-coverageto ensure that flutter coverage rules are passingCHANGELOG.mdfile with your changes (if they affect the user experience)Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
Documentation