Conversation
WalkthroughThis pull request introduces support for managing community guidelines for groups. It includes new UI components, controllers, and data repositories to fetch, display, and update guidelines. Changes encompass routing, localization, and group metadata to fully integrate the community guidelines feature. Additionally, a new entry has been added to the changelog. Changes
Sequence Diagram(s)sequenceDiagram
participant UA as User
participant GA as Group Admin Screen
participant CGS as Community Guidelines Screen
participant CGC as Community Guidelines Controller
participant GMR as Group Metadata Repository
participant Nostr as Nostr SDK
UA ->> GA: Tap "Community Guidelines"
GA ->> CGS: Navigate to CommunityGuidelinesScreen (with groupId)
CGS ->> CGC: Request current guidelines
CGC ->> GMR: Fetch group metadata for groupId
GMR ->> Nostr: Query event for metadata
Nostr -->> GMR: Return metadata (incl. guidelines)
GMR -->> CGC: Provide metadata
CGC -->> CGS: Supply guidelines data
UA ->> CGS: Edit guidelines and tap Save
CGS ->> CGC: Invoke save(new guidelines)
CGC ->> GMR: Update metadata with new guidelines
GMR ->> Nostr: Send update event
Nostr -->> GMR: Acknowledge update
GMR -->> CGC: Confirm success
CGC -->> CGS: Refresh UI with updated guidelines
Suggested labels
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
martindsq
requested review from
bryanmontz,
joshuatbrown,
mplorentz and
pelumy
as code owners
|
The lint checker is complaining about constants in router_consts I didn't modify. I could fix them anyway here, but @bryanmontz already addressed this issue in a different PR. So, let's ignore the failing check. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
lib/features/community_guidelines/community_guidelines_screen.dart (4)
23-27: Consider adding documentation for the TextEditingControllerThe controller is correctly defined, but adding a brief comment about its lifecycle management would improve maintainability.
28-44: Use more descriptive variable names for route argumentsThe generic variable names
argandidcould be more descriptive to enhance readability.- var arg = RouterUtil.routerArgs(context); - if (arg == null || arg is! GroupIdentifier) { - RouterUtil.back(context); - return Container(); - } - final id = arg; + var groupArg = RouterUtil.routerArgs(context); + if (groupArg == null || groupArg is! GroupIdentifier) { + RouterUtil.back(context); + return Container(); + } + final groupId = groupArg;
48-74: Consider extracting AppBar to a separate method for better readabilityThe AppBar configuration is quite lengthy and could be extracted to a dedicated method to improve code organization.
- appBar: AppBar( - leading: const AppbarBackBtnWidget(), - bottom: const AppBarBottomBorder(), - title: Text( - localization.Community_Guidelines, - style: TextStyle( - fontSize: bodyLargeFontSize, - fontWeight: FontWeight.bold, - ), - ), - actions: [ - TextButton( - onPressed: isSaveDisabled ? null : () => _save(id), - style: TextButton.styleFrom( - foregroundColor: accentColor, - disabledForegroundColor: accentColor.withOpacity(0.4), - ), - child: Text( - localization.Save, - style: const TextStyle( - fontSize: 18, - ), - ), - ), - ], - ), + appBar: _buildAppBar(localization, bodyLargeFontSize, accentColor, isSaveDisabled, id),And then add this method:
PreferredSizeWidget _buildAppBar(S localization, double? bodyLargeFontSize, Color accentColor, bool isSaveDisabled, GroupIdentifier id) { return AppBar( leading: const AppbarBackBtnWidget(), bottom: const AppBarBottomBorder(), title: Text( localization.Community_Guidelines, style: TextStyle( fontSize: bodyLargeFontSize, fontWeight: FontWeight.bold, ), ), actions: [ TextButton( onPressed: isSaveDisabled ? null : () => _save(id), style: TextButton.styleFrom( foregroundColor: accentColor, disabledForegroundColor: accentColor.withOpacity(0.4), ), child: Text( localization.Save, style: const TextStyle( fontSize: 18, ), ), ), ], ); }
75-117: Consider extracting the form into a separate methodThe form UI in the body is quite lengthy and could be extracted to a dedicated method for better readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
lib/generated/intl/messages_en.dartis excluded by!**/generated/**lib/generated/intl/messages_es.dartis excluded by!**/generated/**lib/generated/l10n.dartis excluded by!**/generated/**
📒 Files selected for processing (10)
CHANGELOG.md(1 hunks)lib/consts/router_path.dart(1 hunks)lib/data/group_metadata_repository.dart(1 hunks)lib/features/community_guidelines/community_guidelines_controller.dart(1 hunks)lib/features/community_guidelines/community_guidelines_screen.dart(1 hunks)lib/l10n/intl_en.arb(2 hunks)lib/l10n/intl_es.arb(4 hunks)lib/main.dart(2 hunks)lib/router/group/group_admin/group_admin_screen.dart(1 hunks)packages/nostr_sdk/lib/nip29/group_metadata.dart(4 hunks)
🧰 Additional context used
🪛 GitHub Actions: Analyze Project
lib/consts/router_path.dart
[warning] 40-40: The constant name 'QRSCANNER' isn't a lowerCamelCase identifier.
[warning] 41-41: The constant name 'WEBUTILS' isn't a lowerCamelCase identifier.
[warning] 42-42: The constant name 'RELAY_INFO' isn't a lowerCamelCase identifier.
[warning] 43-43: The constant name 'FOLLOWED_TAGS_LIST' isn't a lowerCamelCase identifier.
[warning] 44-44: The constant name 'COMMUNITY_DETAIL' isn't a lowerCamelCase identifier.
[warning] 45-45: The constant name 'FOLLOWED_COMMUNITIES' isn't a lowerCamelCase identifier.
[warning] 46-46: The constant name 'FOLLOWED' isn't a lowerCamelCase identifier.
[warning] 47-47: The constant name 'BOOKMARK' isn't a lowerCamelCase identifier.
[warning] 48-48: The constant name 'FOLLOW_SET_LIST' isn't a lowerCamelCase identifier.
[warning] 49-49: The constant name 'FOLLOW_SET_DETAIL' isn't a lowerCamelCase identifier.
[warning] 50-50: The constant name 'FOLLOW_SET_FEED' isn't a lowerCamelCase identifier.
[warning] 51-51: The constant name 'NWC_SETTING' isn't a lowerCamelCase identifier.
[warning] 52-52: The constant name 'GROUP_ADMIN' isn't a lowerCamelCase identifier.
[warning] 53-53: The constant name 'GROUP_LIST' isn't a lowerCamelCase identifier.
[warning] 54-54: The constant name 'GROUP_DETAIL' isn't a lowerCamelCase identifier.
[warning] 55-55: The constant name 'GROUP_EDIT' isn't a lowerCamelCase identifier.
[warning] 56-56: The constant name 'GROUP_MEMBERS' isn't a lowerCamelCase identifier.
[warning] 57-57: The constant name 'GROUP_INFO' isn't a lowerCamelCase identifier.
🔇 Additional comments (17)
CHANGELOG.md (1)
22-22: Good addition to the changelogThe new entry correctly documents the addition of Community Guidelines to groups and includes the appropriate issue reference.
lib/consts/router_path.dart (1)
37-38:Details
✅ Verification successful
Good implementation of new route path
The new constant is properly documented and follows the same pattern as other recent additions to the file like
onboardingandpushNotificationTest.I notice there's a mix of naming conventions in this file (some constants use UPPER_SNAKE_CASE while newer ones use lowerCamelCase). This is confirmed by the lint warnings. While your addition follows the newer convention correctly, you might want to consider a future task to standardize these constants.
🏁 Script executed:
#!/bin/bash # Count the number of constants using each naming convention echo "Constants using UPPER_SNAKE_CASE:" grep -E 'static const String [A-Z_]+' lib/consts/router_path.dart | wc -l echo "Constants using lowerCamelCase:" grep -E 'static const String [a-z][a-zA-Z]*' lib/consts/router_path.dart | wc -lLength of output: 313
New Route Constant Approved – Note on Naming Conventions
- The new
communityGuidelinesconstant is documented and implemented correctly following the lowerCamelCase style, similar to other recent additions likeonboardingandpushNotificationTest.- Verification shows that while only 3 constants use lowerCamelCase, there are 39 legacy constants in UPPER_SNAKE_CASE, which explains the lint warnings.
- Consider scheduling a future task to standardize naming conventions across the file.
lib/main.dart (2)
63-63: Good import for the new screenThe import statement is properly placed with the other feature imports.
467-469: Good implementation of route handlerThe route is properly registered and follows the same pattern as other routes in the application.
lib/router/group/group_admin/group_admin_screen.dart (1)
216-223:Details
✅ Verification successful
Good implementation of navigation to Community Guidelines
The navigation row is implemented correctly and follows the same pattern as the existing Members navigation row. The groupId is properly passed to the new route.
Let's verify that the localization key for "Community_Guidelines" exists in the localization files:
🏁 Script executed:
#!/bin/bash # Check if the Community_Guidelines localization key exists echo "Checking for Community_Guidelines in English localization file:" grep -r "Community_Guidelines" lib/l10n/ echo "Checking other Community_ related strings:" grep -r "Community_" lib/l10n/Length of output: 1219
Navigation to Community Guidelines Verified
The localization key
"Community_Guidelines"is confirmed to exist (in bothintl_en.arbandintl_es.arb), and the navigation row inlib/router/group/group_admin/group_admin_screen.dart(lines 216-223) is implemented consistently with the Members navigation row. The group identifier (groupId) is correctly passed to the new route.lib/features/community_guidelines/community_guidelines_screen.dart (2)
1-21: LGTM: Clean implementation of the CommunityGuidelinesScreen classThe ConsumerStatefulWidget implementation is well structured with appropriate imports and documentation.
121-153: Well-implemented save method with proper error handlingGood job on implementing comprehensive error handling with appropriate user feedback and retry options.
lib/l10n/intl_en.arb (3)
4-5: LGTM: Properly added error handling localizationsThese new keys for error handling will ensure a consistent user experience during error scenarios.
7-7: LGTM: Properly added Community Guidelines localizationThe localization key for the screen title is correctly defined.
335-338: LGTM: Properly added input and error localizationsThe localization keys for the input hint and save error messages are correctly defined.
lib/l10n/intl_es.arb (3)
33-33: LGTM: Spanish translation for Community Guidelines addedThe Spanish translation for the Community Guidelines title has been correctly added.
64-64: LGTM: Spanish translations for description and guidelines properly addedThe translations for Description and Community Guidelines input are properly implemented.
Also applies to: 72-73
181-181: LGTM: Spanish translations for action buttons and errors addedThe translations for Retry, Save, and Save failed messages are correctly implemented.
Also applies to: 190-191
packages/nostr_sdk/lib/nip29/group_metadata.dart (2)
11-13: LGTM: Well-documented new property for community guidelinesThe community guidelines property is properly documented with clear purpose.
25-25: LGTM: Constructor updated to include community guidelines parameterThe constructor has been properly updated to accept the new property.
lib/features/community_guidelines/community_guidelines_controller.dart (1)
36-39: Consider handling potential exceptions during build.The
buildmethod directly calls_fetchCommunityGuidelines(arg)without any error handling. If the repository throws an exception, it will propagate unhandled. You might want to wrap this in atry/catchor rely on Riverpod's error state to provide a more informative UI message for the user.lib/data/group_metadata_repository.dart (1)
68-82: Verify handling of marker collisions.You're slicing the
aboutfield at the first occurrence of# Community Guidelines. If the marker appears multiple times, only the first section is assigned toaboutand the remainder tocommunityGuidelines. Confirm whether this is intentional or if additional occurrences need processing.
@martindsq Could you update the placeholder text to use the "Dimmed" color variable, which is #8D7EAB on dark and #A68782 on light mode? |
Done. Here's an updated screenshot.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/features/community_guidelines/community_guidelines_screen.dart (2)
42-45: Consider adding a check for unsaved changes when navigating backWhen the user navigates back without saving changes, there's no confirmation dialog to warn them of potential data loss.
Consider overriding the back button behavior in the app bar or wrapping the Scaffold with a WillPopScope to check for unsaved changes:
if (arg == null || arg is! GroupIdentifier) { - RouterUtil.back(context); + // Consider checking for unsaved changes before navigating back + _handleBackNavigation(); return Container(); }Then add a method to handle back navigation:
void _handleBackNavigation() { final String currentText = _descriptionController.text; final String originalText = ref.read(communityGuidelinesControllerProvider(id)).value ?? ''; if (currentText != originalText) { // Show confirmation dialog showDialog( context: context, builder: (context) => AlertDialog.adaptive( title: Text(S.of(context).Unsaved_Changes), content: Text(S.of(context).Discard_Changes_Confirmation), actions: [ TextButton( child: Text(S.of(context).Discard), onPressed: () { Navigator.of(context).pop(); RouterUtil.back(context); }, ), TextButton( child: Text(S.of(context).Cancel), onPressed: () => Navigator.of(context).pop(), ), ], ), ); } else { RouterUtil.back(context); } }Note: This would require adding the appropriate localization strings.
133-153: Enhance error feedback with more specific detailsThe current error dialog just shows a generic "Save failed" message without providing details about what went wrong.
Consider passing the error details from the controller to the UI:
- final result = await controller.save(_descriptionController.text); + final saveResult = await controller.save(_descriptionController.text); if (!mounted) return; - if (result) { + if (saveResult.success) { RouterUtil.back(context); } else { showDialog( context: context, barrierDismissible: true, builder: (context) => AlertDialog.adaptive( title: Text(localization.Error), - content: Text(localization.Save_failed), + content: Text(saveResult.errorMessage ?? localization.Save_failed), actions: [This assumes you modify the controller to return a result object with success and error message fields rather than just a boolean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/features/community_guidelines/community_guidelines_screen.dart(1 hunks)
🔇 Additional comments (2)
lib/features/community_guidelines/community_guidelines_screen.dart (2)
104-105: LGTM! Correct use of dimmed color for hint textThe hint text correctly uses the dimmed color as requested in the PR comments. This provides proper contrast and follows the design specifications.
1-156: Overall the implementation is well-structured and follows good practicesThe code is well-organized, properly documented, and follows Flutter best practices. The separation of UI and business logic (via the controller) is well done, and the handling of different states (loading, error, data) is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/features/community_guidelines/community_guidelines_screen.dart (1)
123-127: TextEditingController properly disposedGreat job implementing the dispose method to clean up the TextEditingController, preventing potential memory leaks. This follows Flutter best practices.
🧹 Nitpick comments (1)
lib/features/community_guidelines/community_guidelines_screen.dart (1)
28-121: Consider refactoring the build method for better readabilityThe build method is quite long (90+ lines). Consider breaking it down into smaller widget methods for better readability and maintainability, such as:
_buildAppBar()_buildContentBody(String value)_buildForm(String value)This would make the code more modular and easier to maintain.
Example refactoring:
@override Widget build(BuildContext context) { // Theme setup and arg validation code final id = arg; final controller = ref.watch(communityGuidelinesControllerProvider(id)); return Scaffold( appBar: _buildAppBar(controller.isLoading || controller.hasError, id), body: controller.when( data: (value) => _buildContentBody(value), error: (error, stackTrace) => Center(child: ErrorWidget(error)), loading: () => const Center(child: CircularProgressIndicator()), ), ); } PreferredSizeWidget _buildAppBar(bool isSaveDisabled, GroupIdentifier id) { // Existing app bar code } Widget _buildContentBody(String value) { _descriptionController.text = value; return SingleChildScrollView( child: Column( // Existing column code ), ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/features/community_guidelines/community_guidelines_screen.dart(1 hunks)
🔇 Additional comments (3)
lib/features/community_guidelines/community_guidelines_screen.dart (3)
23-27: TextEditingController properly initializedGood job on implementing the TextEditingController with a descriptive comment. I see you've also added the dispose method as suggested in the previous review.
129-161: Robust error handling in save methodThe _save method handles errors appropriately by:
- Checking if the widget is still mounted after the async operation
- Providing a user-friendly error dialog
- Offering retry functionality
- Including proper navigation in success and failure cases
This shows good attention to user experience and robustness.
1-162: Overall implementation is well-structured and functionalThe CommunityGuidelinesScreen implementation is well-structured with:
- Clear separation of UI and logic
- Proper state management with Riverpod
- Appropriate error handling and loading states
- Good use of localization
- Responsive UI that adapts to theme settings
The code follows Flutter best practices and provides a good user experience for managing community guidelines.
martindsq
added
skip-linting
|
👀 |
pelumy
left a comment
pelumy
left a comment
There was a problem hiding this comment.
Thanks for doing thia Martin! A lot of code improvements in this PR. I also love the feature folder.
The PR works great. Only UI issue I found is that there should be a line between the menus.
Figma: https://www.figma.com/design/MB4AOVkZ2wt3IfI4aMAusZ/Plur?node-id=618-3261&m=dev
It currently looks like:
But I don't think it is a blocker.
3 participants
Issues covered
#264
Description
A description of the changes proposed in the pull request. Explain what it does to give the reviewer some context for what they're reviewing.
How to test
Screenshots/Video
Post screenshots or video showing your changes, ideally showing how the app worked before and after these changes. Delete this section if this PR contains no visual changes.
@Chardot Please review these UI changes to be sure they match the design.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation