refactor: removed dummy data from various components #657
Conversation
|
🎉 Welcome @negimox!
We appreciate your contribution! 🚀 |
WalkthroughReplaced a hardcoded Appwrite recovery URL with a dynamic one, added a NotificationsController with reactive loading/error/delete handling, changed Participant.fromJson to accept an optional placeholder and use a constants-based fallback, and removed several unused mock/top-level data and a removed UI class. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as NotificationsScreen (UI)
participant Ctrl as NotificationsController
participant API as Backend/API (placeholder)
UI->>Ctrl: onInit / build -> controller created
Ctrl->>API: fetchNotifications() (async)
activate API
API-->>Ctrl: notifications list / error
deactivate API
Ctrl-->>UI: updates RxList / isLoading / error (Obx reacts)
Note right of UI: User actions
UI->>Ctrl: pull-to-refresh -> refreshNotifications()
Ctrl->>API: fetchNotifications()
UI->>Ctrl: delete button tapped -> deleteNotification(notification)
Ctrl-->>Ctrl: remove item from RxList (and call backend placeholder)
Ctrl->>API: delete request (optional)
API-->>Ctrl: delete success/failure
Ctrl-->>UI: updated RxList / possibly error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/controllers/authentication_controller.dart(1 hunks)lib/models/participant.dart(2 hunks)lib/views/screens/home_screen.dart(0 hunks)lib/views/screens/no_room_screen.dart(0 hunks)lib/views/screens/notifications_screen.dart(1 hunks)
💤 Files with no reviewable changes (2)
- lib/views/screens/no_room_screen.dart
- lib/views/screens/home_screen.dart
🔇 Additional comments (1)
lib/models/participant.dart (1)
1-2: Verify initialization order: Participant.fromJson depends on ThemeController registration.Participant.fromJson calls
Get.find<ThemeController>()(line 27) to retrieve a placeholder image URL (line 31). This creates a runtime dependency: if Participant deserialization occurs before ThemeController is registered viaGet.put(), the app will crash with "GetxController not found" error.While ThemeController is registered early in auth_splash_bindings.dart:19 and main.dart:79, relying on
Get.find()within a model constructor is a fragile architectural pattern that tightly couples the model layer to GetX service locator. Consider refactoring to pass the placeholder URL directly to the constructor or factory method instead of fetching it from the controller during deserialization.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
lib/views/screens/notifications_screen.dart (2)
11-11: Consider usingGet.find()with lazy registration instead ofGet.put()in field initializer.Using
Get.put()directly in a StatelessWidget's field initializer can create multiple controller instances if the widget is rebuilt or instantiated multiple times during navigation. Consider registering the controller in a binding or usingGet.put()withpermanent: true, or switch toGet.find()after ensuring the controller is registered elsewhere.- final NotificationsController controller = Get.put(NotificationsController()); + final NotificationsController controller = Get.put(NotificationsController(), permanent: true);Alternatively, register in a binding and use
Get.find<NotificationsController>().
51-69: Consider sanitizing the error message before displaying to users.Displaying
controller.error.value!directly may expose technical details (stack traces, internal messages) to end users. Consider showing a user-friendly message instead.Text( - controller.error.value!, + AppLocalizations.of(context)!.somethingWentWrong, style: const TextStyle(fontSize: 16), ),lib/controllers/notifications_controller.dart (2)
15-46: Good placeholder implementation with clear documentation.The TODO comments provide excellent guidance for future backend integration, including a complete code example. The simulated delay helps test the loading state UI.
Would you like me to open an issue to track the backend integration for notifications?
58-62: Add error handling for future backend integration.When the backend call is implemented, deletion could fail. Consider adding try-catch now to establish the pattern and prevent UI inconsistencies.
/// Delete a notification Future<void> deleteNotification(NotificationModel notification) async { - // TODO: Implement backend call to delete notification - notifications.remove(notification); + try { + // TODO: Implement backend call to delete notification + // await apiService.deleteNotification(notification.id); + notifications.remove(notification); + } on Exception catch (e) { + error.value = e.toString(); + // Optionally re-fetch to restore consistent state + // await fetchNotifications(); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/controllers/authentication_controller.dart(1 hunks)lib/controllers/notifications_controller.dart(1 hunks)lib/models/participant.dart(2 hunks)lib/views/screens/notifications_screen.dart(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/controllers/authentication_controller.dart
🔇 Additional comments (7)
lib/views/screens/notifications_screen.dart (3)
43-112: Good implementation of reactive state handling.The Obx wrapper correctly handles loading, error, empty, and data states with appropriate UI feedback. The RefreshIndicator integration and delete functionality are well-structured.
118-126: Clean extension of NotificationTile API.The optional
onDeletecallback is a good approach, allowing the tile to be reusable in contexts where deletion isn't needed.
204-211: LGTM!Conditional rendering of the delete button based on
onDeletepresence is correctly implemented.lib/controllers/notifications_controller.dart (2)
1-7: Well-structured reactive state management.Good use of
Rxn<String>for nullable error state andRxListfor the notifications collection. The observable declarations follow GetX conventions.
53-56: Placeholder noted.The
markAsReadplaceholder is acceptable. Consider adding it to the tracking issue for backend integration.lib/models/participant.dart (2)
1-1: LGTM!The constants import is necessary for accessing the placeholder URL constant used in the fallback logic.
25-26: Successfully addresses the previous architectural concern.The addition of the optional
placeholderUrlparameter resolves the tight coupling toThemeControllerthat was flagged in the previous review. This approach provides flexibility while maintaining clean separation between the model and presentation layers.
| // Use provided placeholderUrl or fall back to chapter placeholder | ||
| // In production, the backend should always provide dpUrl | ||
| dpUrl = json['dpUrl'] ?? | ||
| (placeholderUrl ?? chapterCoverImagePlaceholderUrl); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find all Participant.fromJson call sites and check context
rg -n 'Participant\.fromJson\s*\(' -A 3 -B 2Repository: AOSSIE-Org/Resonate
Length of output: 420
🏁 Script executed:
# Search for user/profile placeholder constants in constants
rg -n 'user.*[Pp]laceholder|profile.*[Pp]laceholder' --type dartRepository: AOSSIE-Org/Resonate
Length of output: 1959
🏁 Script executed:
# Check what placeholders are defined in constants file
fd -e dart -name '*constant*' | head -20Repository: AOSSIE-Org/Resonate
Length of output: 293
🏁 Script executed:
# Find where chapterCoverImagePlaceholderUrl is defined and used
rg -n 'chapterCoverImagePlaceholderUrl' --type dartRepository: AOSSIE-Org/Resonate
Length of output: 707
🏁 Script executed:
# Search for Participant.fromJson calls (usage, not definition)
rg -n 'Participant\.fromJson' --type dart -A 4 | grep -v "^--$"Repository: AOSSIE-Org/Resonate
Length of output: 389
🏁 Script executed:
# Check the participant.dart file more closely to see the full constructor and field types
cat -n lib/models/participant.dart | head -50Repository: AOSSIE-Org/Resonate
Length of output: 1899
🏁 Script executed:
# Search for where Participant objects are created from JSON, particularly in API/networking code
rg -n 'fromJson' lib/models/participant.dart -B 5 -A 8Repository: AOSSIE-Org/Resonate
Length of output: 606
🏁 Script executed:
# Check if there are any API response handlers that call Participant.fromJson
rg -n 'Participant\(' --type dart -B 3 -A 3 | head -60Repository: AOSSIE-Org/Resonate
Length of output: 4421
🏁 Script executed:
# Verify the definition of userProfileImagePlaceholderUrl in theme_controller
cat -n lib/themes/theme_controller.dart | sed -n '20,35p'Repository: AOSSIE-Org/Resonate
Length of output: 737
Use dedicated user profile placeholder instead of chapter placeholder.
The fallback logic uses chapterCoverImagePlaceholderUrl for the user profile image field (dpUrl), but a dedicated userProfileImagePlaceholderUrl constant already exists in theme_controller.dart. Replace the fallback with the appropriate constant to maintain semantic consistency:
dpUrl = json['dpUrl'] ??
(placeholderUrl ?? userProfileImagePlaceholderUrl);Import userProfileImagePlaceholderUrl from the theme controller or use it through the theme controller instance if that's the established pattern in the codebase.
🤖 Prompt for AI Agents
In lib/models/participant.dart around lines 30 to 33, the dpUrl fallback
incorrectly uses chapterCoverImagePlaceholderUrl; change the fallback to use the
dedicated userProfileImagePlaceholderUrl instead so profile images use the
correct semantic placeholder, and update imports or access pattern to import
userProfileImagePlaceholderUrl from theme_controller.dart (or reference it via
the theme controller instance consistent with project conventions).
| Text( | ||
| 'No notifications yet', | ||
| style: const TextStyle( | ||
| fontSize: 18, | ||
| color: Colors.grey, | ||
| ), | ||
| ), |
There was a problem hiding this comment.
Localize the empty state message.
The "No notifications yet" string is hardcoded in English while other strings in the file use AppLocalizations. This will cause inconsistent language display for non-English users.
Text(
- 'No notifications yet',
+ AppLocalizations.of(context)!.noNotifications,
style: const TextStyle(
fontSize: 18,
color: Colors.grey,
),
),Note: You'll need to add the corresponding key to your localization files.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/views/screens/notifications_screen.dart around lines 83 to 89 the
empty-state string "No notifications yet" is hardcoded; replace it with a
localized lookup (e.g. AppLocalizations.of(context)!.noNotificationsYet or your
project's equivalent getter) so the message follows the app's i18n pattern, and
add the corresponding key/value to the localization files (ARB/JSON) for all
supported languages then regenerate localization artifacts.
|
Closing as linked issue is already assigned to another contributor |
|
✅ PR Closed - Thank You, @negimox!
We appreciate your effort and look forward to more contributions from you! 🤝 |
2 participants
Description
This PR removes all dummy/mock data from the Resonate codebase that was previously used for testing and development purposes but is no longer needed in production. It is for issue #555 .
Total Impact:
~220 lines of dummy data removed
5 files modified
0 new errors or warnings introduced
No breaking changes
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Static Analysis: Ran flutter analyze - confirmed no new errors or warnings introduced
Code Review: Verified all removed data was:
Never referenced in the codebase
Not used in any active features
Purely test/development artifacts
Import Verification: Ensured all unused imports were removed
Compilation Check: Verified all modified files compile without errors
Test Configuration:
Platform: macOS
Flutter Version: Latest stable
Branch: 555/Remove_Dummy_Data
Checklist:
Maintainer Checklist
Summary by CodeRabbit
New Features
Improvements
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.