flutter: ShareService — unify SharePlus usage + micro-cleanups #83
Conversation
Summary of ChangesHello @zensgit, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request streamlines the application's sharing functionality by standardizing its interaction with the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull Request Overview
Unifies ShareService to use SharePlus.instance.share(ShareParams) consistently while preserving the _doShare hook for testing. Includes micro cleanups such as pre-capturing ScaffoldMessenger instances to avoid context warnings and removing unused variables/stubs.
- Standardized sharing mechanism using SharePlus.instance.share(ShareParams)
- Pre-captured ScaffoldMessenger instances to avoid async context usage warnings
- Removed unused variables, imports, and stub classes
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| jive-flutter/lib/widgets/source_badge.dart | Removed unused cs variable |
| jive-flutter/lib/widgets/qr_code_generator.dart | Updated QR code styling from foregroundColor to eyeStyle and dataModuleStyle |
| jive-flutter/lib/widgets/custom_theme_editor.dart | Added lint suppressions for context usage after async operations |
| jive-flutter/lib/widgets/common/right_click_copy.dart | Pre-captured messenger to avoid context warnings |
| jive-flutter/lib/widgets/batch_operation_bar.dart | Pre-captured navigator and messenger references |
| jive-flutter/lib/utils/image_utils.dart | Removed unused image extension checking logic |
| jive-flutter/lib/ui/components/transactions/transaction_list.dart | Removed unused currency_provider import |
| jive-flutter/lib/ui/components/loading/loading_widget.dart | Removed unused theme variable |
| jive-flutter/lib/ui/components/dashboard/dashboard_overview.dart | Updated type references and removed comment |
| jive-flutter/lib/ui/components/dashboard/account_overview.dart | Commented out unused groupedAccounts variable |
| jive-flutter/lib/ui/components/cards/account_card.dart | Removed unused currency formatting method and variables |
| jive-flutter/lib/ui/components/buttons/secondary_button.dart | Added const to EdgeInsets constructor |
| jive-flutter/lib/ui/components/budget/budget_progress.dart | Updated to ConsumerWidget and added currency provider |
| jive-flutter/lib/ui/components/accounts/account_list.dart | Updated account type mapping and constructor calls |
| jive-flutter/lib/services/share_service.dart | Unified ShareService to use SharePlus, pre-captured messenger, removed unused stubs |
| jive-flutter/lib/core/network/interceptors/retry_interceptor.dart | Removed unused _lastGlobalFailure variable |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| ]; | ||
|
|
||
| // Allow URLs without extensions (many CDNs don't use them) | ||
| // Check for common image extensions// Allow URLs without extensions (many CDNs don't use them) |
There was a problem hiding this comment.
Missing space between 'extensions' and '// Allow'. Should be '// Check for common image extensions // Allow URLs without extensions (many CDNs don't use them)'.
| // Check for common image extensions// Allow URLs without extensions (many CDNs don't use them) | |
| // Check for common image extensions // Allow URLs without extensions (many CDNs don't use them) |
| ), | ||
| ), | ||
| const SizedBox( | ||
| SizedBox( |
There was a problem hiding this comment.
Removing 'const' from SizedBox makes it less efficient. Since SizedBox has a constant width (45), it should remain const.
| SizedBox( | |
| const SizedBox( |
|
|
||
| // Model<->UI AccountType adapter | ||
| // Map model.AccountType (checking/savings/creditCard/loan/...) to local grouping (asset/liability) | ||
| AccountType _toUiAccountType(model.AccountType t) { |
There was a problem hiding this comment.
The helper functions _toUiAccountType and _matchesLocalType are defined outside of any class scope, making them top-level private functions. These should be methods within the AccountList class or moved to a separate utility class.
| onPressed: onShareMore ?? | ||
| () async { | ||
| await Share.share('$content\n\n$url'); | ||
| await SharePlus.instance.share(ShareParams(text: '$content\n\n${url ?? ''}')); |
There was a problem hiding this comment.
The null-aware operator on url suggests it might be null, but this could result in the text ending with an empty string. Consider handling null urls more explicitly, such as only appending the url if it's not null.
| await SharePlus.instance.share(ShareParams(text: '$content\n\n${url ?? ''}')); | |
| await SharePlus.instance.share( | |
| ShareParams( | |
| text: url != null ? '$content\n\n$url' : content, | |
| ), | |
| ); |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request primarily focuses on refactoring the ShareService to unify its implementation around SharePlus and includes various micro-cleanups across the codebase. The changes are generally positive, improving code quality by removing unused code, centralizing currency formatting, and addressing lint warnings. I've provided a few suggestions to enhance maintainability and correctness further. These include simplifying a helper function, reconsidering the use of a dynamic type to maintain type safety, addressing a validation function that has become too permissive, and applying a more robust pattern for handling asynchronous gaps involving BuildContext.
| // Check for common image extensions// Allow URLs without extensions (many CDNs don't use them) | ||
| // but validate the URL structure | ||
| return uri.host.isNotEmpty; |
There was a problem hiding this comment.
The validation logic in isValidImageUrl has been weakened significantly. It now only checks for a valid URL scheme and a non-empty host, which means it will return true for almost any valid URL (e.g., http://google.com), not just image URLs. This makes the function name misleading and could lead to incorrect assumptions about the URL's content.
Consider either restoring a more robust check (e.g., checking for common image extensions, while acknowledging its limitations for CDN URLs) or renaming the function to better reflect its actual purpose, such as isValidHttpUrl.
| bool _matchesLocalType(AccountType localType, model.AccountType modelType) { | ||
| final isLiability = modelType == model.AccountType.creditCard || modelType == model.AccountType.loan; | ||
| if (localType == AccountType.liability) return isLiability; | ||
| return !isLiability; | ||
| } |
There was a problem hiding this comment.
The implementation of _matchesLocalType can be simplified by reusing the logic from _toUiAccountType. This avoids duplicating the logic for what constitutes a liability account and makes the code more maintainable and less prone to errors if the definition of a liability account changes in the future.
bool _matchesLocalType(AccountType localType, model.AccountType modelType) {
return _toUiAccountType(modelType) == localType;
}| final List<QuickActionData> quickActions; | ||
| final List<TransactionData> recentTransactions; | ||
| final List<BalancePoint> balanceData; | ||
| final List<dynamic> quickActions; |
There was a problem hiding this comment.
The type of quickActions in DashboardData has been changed to List<dynamic>. Using dynamic loses type safety, which can lead to runtime errors and makes the code harder to maintain. It would be better to use a specific type. If QuickActionData was intentionally removed, consider defining a new simple data class for the actions or using Map<String, dynamic> if the structure is consistent and well-defined.
…nce.share with ShareParams; add cross_file import
…nd leftover stub; fix const usage; migrate invite dialog share to SharePlus.instance
…used groupedAccounts; const-safe padding in SecondaryButton
…e type infos; keep UI enums for grouping visuals
…SubType and adapt mapping; use AccountCard.fromAccount everywhere
…olve prefixed enum typos
…pe; correct adapters to model.AccountType mapping
…um declarations to satisfy analyzer
…ing in SecondaryButton
…oan => liability; others => asset) and total filter by matcher
…s); keep _doShare hook for tests; replace final Share.share usage in ShareDialog
…e annotations, remove unused locals warnings)
zensgit
force-pushed
the
flutter/share-service-shareplus
branch
from
bd79d99 to
959e66a
Compare
1 participant
Validation: tests green; analyzer OK for changed areas