flutter: context cleanup batch 2 — TemplateAdminPage context-safety #63
Conversation
…nce.share/shareXFiles in QR + ShareService; remove local stub
…hare_plus directly
…g and DeleteFamilyDialog
…or) + fix const-eval in common widgets
…pture and fix duplicates
…ignores; fix template onCancel and batch dialogs snackbars
…ait messenger capture
… dashboard types and stability
…plateAdminPage; mounted guards; remove ignores
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 focuses on enhancing the stability and maintainability of the Flutter application by addressing common 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
This PR improves context safety in Flutter code by addressing use_build_context_synchronously warnings through capturing context-dependent objects before async operations and using mounted guards. The primary focus is on the TemplateAdminPage with systematic cleanup of analyzer warnings across multiple files.
- Captured Navigator/ScaffoldMessenger before awaits and used mounted guards in TemplateAdminPage
- Applied context-safety patterns to multiple dialog and widget files throughout the codebase
- Removed redundant analyzer ignore comments and cleaned up unused imports/code
Reviewed Changes
Copilot reviewed 39 out of 43 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| jive-flutter/lib/screens/admin/template_admin_page.dart | Applied context-safety pattern by capturing messenger/navigator before awaits and using mounted guards |
| jive-flutter/lib/widgets/theme_share_dialog.dart | Updated to capture ScaffoldMessenger before async operations and use mounted guards |
| jive-flutter/lib/widgets/dialogs/accept_invitation_dialog.dart | Removed unused imports and captured context objects before async calls |
| jive-flutter/lib/widgets/batch_operation_bar.dart | Applied context-safety pattern with captured navigator/messenger and mounted guards |
| jive-flutter/lib/services/share_service.dart | Cleaned up stub code and improved context handling for clipboard operations |
| jive-flutter/lib/widgets/states/loading_indicator.dart | Removed const from SizedBox constructors with non-constant parameters |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // ignore: use_build_context_synchronously | ||
| navigator.pop(); | ||
| widget.onConfirm(); | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| // ignore: use_build_context_synchronously |
There was a problem hiding this comment.
The analyzer ignore comments are unnecessary since the Navigator and ScaffoldMessenger were captured before the async operation. These ignores should be removed as the context safety issue has been properly resolved.
| // ignore: use_build_context_synchronously | ||
| navigator.pop(); | ||
| widget.onConfirm(); | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| // ignore: use_build_context_synchronously |
There was a problem hiding this comment.
Similar to the previous comment, these analyzer ignore directives are redundant since the context objects were properly captured before the async operation. Remove these ignore comments.
| final result = await _authService.wechatLogin(); | ||
|
|
||
| if (!context.mounted) return; | ||
| // ignore: use_build_context_synchronously |
There was a problem hiding this comment.
The ignore comment is misplaced. The ScaffoldMessenger is being captured at the right time (before the async operation), so this ignore comment should be removed. The actual usage of messenger later doesn't need ignoring since it was properly captured.
| // ignore: use_build_context_synchronously |
![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 focuses on improving code quality and stability, particularly by addressing use_build_context_synchronously warnings in the Flutter codebase. The changes to capture BuildContext dependent objects like Navigator and ScaffoldMessenger before await calls are excellent and follow best practices. The Rust changes also improve logging and error handling.
However, I've identified a few instances where the use_build_context_synchronously fix was applied incorrectly, potentially hiding the issue with an ignore comment. There are also some minor regressions and opportunities for more idiomatic code. Additionally, some build artifacts have been included in the commit, which should be added to .gitignore.
| if (!context.mounted) return; | ||
| // ignore: use_build_context_synchronously | ||
| final messenger = ScaffoldMessenger.of(context); |
There was a problem hiding this comment.
This fix for use_build_context_synchronously is incorrect. The ScaffoldMessenger should be captured before the await call to ensure the BuildContext is still valid. The ignore comment is hiding a potential runtime error. Also, it's better to use the mounted property directly in a StatefulWidget's state.
| if (!context.mounted) return; | |
| // ignore: use_build_context_synchronously | |
| final messenger = ScaffoldMessenger.of(context); | |
| final messenger = ScaffoldMessenger.of(context); | |
| final result = await _authService.wechatLogin(); | |
| if (!mounted) return; |
| await Clipboard.setData(ClipboardData(text: text)); | ||
| if (context.mounted) { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| SnackBar( | ||
| content: Text(message ?? '已复制到剪贴板'), | ||
| duration: const Duration(seconds: 2), | ||
| ), | ||
| ); | ||
| } | ||
| messenger.showSnackBar( | ||
| SnackBar( | ||
| content: Text(message ?? '已复制到剪贴板'), | ||
| duration: const Duration(seconds: 2), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
The if (context.mounted) check was removed. It's important to keep this check after an await call to prevent errors if the widget is disposed before the async operation completes. While messenger is captured before the await, the check is still necessary before using it.
| await Clipboard.setData(ClipboardData(text: text)); | |
| if (context.mounted) { | |
| ScaffoldMessenger.of(context).showSnackBar( | |
| SnackBar( | |
| content: Text(message ?? '已复制到剪贴板'), | |
| duration: const Duration(seconds: 2), | |
| ), | |
| ); | |
| } | |
| messenger.showSnackBar( | |
| SnackBar( | |
| content: Text(message ?? '已复制到剪贴板'), | |
| duration: const Duration(seconds: 2), | |
| ), | |
| ); | |
| await Clipboard.setData(ClipboardData(text: text)); | |
| if (!context.mounted) return; | |
| messenger.showSnackBar( | |
| SnackBar( | |
| content: Text(message ?? '已复制到剪贴板'), | |
| duration: const Duration(seconds: 2), | |
| ), | |
| ); |
| if (!mounted) return; | ||
| Navigator.pop(context); | ||
| final navigator = Navigator.of(context); | ||
| final messenger = ScaffoldMessenger.of(context); | ||
| // ignore: use_build_context_synchronously | ||
| navigator.pop(); | ||
| widget.onConfirm(); | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| // ignore: use_build_context_synchronously | ||
| messenger.showSnackBar( | ||
| SnackBar( | ||
| content: Text('已移动 ${widget.selectedIds.length} 个分类'), | ||
| ), |
There was a problem hiding this comment.
This fix for use_build_context_synchronously is incorrect. The navigator and messenger are captured after the await call, and the problem is hidden with an ignore comment. These should be captured before the await to ensure the BuildContext is still valid when they are used.
final navigator = Navigator.of(context);
final messenger = ScaffoldMessenger.of(context);
await provider.batchMoveCategories(
widget.selectedIds,
_targetParentId,
);
if (!mounted) return;
navigator.pop();
widget.onConfirm();
messenger.showSnackBar(
SnackBar(
content: Text('已移动 ${widget.selectedIds.length} 个分类'),
),
);| } | ||
|
|
||
| Navigator.pop(context); | ||
| final navigator = Navigator.of(context); | ||
| final messenger = ScaffoldMessenger.of(context); | ||
| // ignore: use_build_context_synchronously | ||
| navigator.pop(); | ||
| widget.onConfirm(); | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| // ignore: use_build_context_synchronously | ||
| messenger.showSnackBar( | ||
| SnackBar( | ||
| content: Text('已转换 ${widget.selectedIds.length} 个分类为标签'), | ||
| ), |
There was a problem hiding this comment.
Similar to the other async operation in this file, this fix for use_build_context_synchronously is incorrect. The navigator and messenger are captured after the await call, and the problem is hidden with an ignore comment. These should be captured before the await to ensure the BuildContext is still valid.
}
final navigator = Navigator.of(context);
final messenger = ScaffoldMessenger.of(context);
navigator.pop();
widget.onConfirm();
messenger.showSnackBar(
SnackBar(
content: Text('已转换 ${widget.selectedIds.length} 个分类为标签'),
),
);| let family = match family_service.create_family(user_id, family_request).await { | ||
| Ok(f) => f, | ||
| Err(e) => { | ||
| tracing::error!(target: "auth_service", error = ?e, user_id = %user_id, "register_with_family: create_family failed"); | ||
| return Err(e); | ||
| } | ||
| }; |
There was a problem hiding this comment.
While this match block works for logging the error, a more idiomatic and concise way to handle this in Rust is to use the map_err combinator. This avoids the explicit match and keeps the error-propagation flow of the ? operator, making the code cleaner.
let family = family_service.create_family(user_id, family_request).await
.map_err(|e| {
tracing::error!(target: "auth_service", error = ?e, user_id = %user_id, "register_with_family: create_family failed");
e
})?;| isOverBudget | ||
| ? '超支 ${ref.read(currencyProvider.notifier).formatCurrency(-remaining, ref.read(baseCurrencyProvider).code)}' | ||
| : '剩余 ${ref.read(currencyProvider.notifier).formatCurrency(remaining, ref.read(baseCurrencyProvider).code)}', | ||
| ? '超支 ${remaining.abs().toStringAsFixed(2)}' | ||
| : '剩余 ${remaining.toStringAsFixed(2)}', |
There was a problem hiding this comment.
This change removes the currency formatting and replaces it with a hardcoded toStringAsFixed(2). This is a regression, as it loses the currency symbol and locale-specific formatting provided by currencyProvider. It's better to use the currency provider to ensure consistent formatting across the app.
| isOverBudget | |
| ? '超支 ${ref.read(currencyProvider.notifier).formatCurrency(-remaining, ref.read(baseCurrencyProvider).code)}' | |
| : '剩余 ${ref.read(currencyProvider.notifier).formatCurrency(remaining, ref.read(baseCurrencyProvider).code)}', | |
| ? '超支 ${remaining.abs().toStringAsFixed(2)}' | |
| : '剩余 ${remaining.toStringAsFixed(2)}', | |
| isOverBudget | |
| ? '超支 ${ref.read(currencyProvider.notifier).formatCurrency(remaining.abs(), ref.read(baseCurrencyProvider).code)}' | |
| : '剩余 ${ref.read(currencyProvider.notifier).formatCurrency(remaining, ref.read(baseCurrencyProvider).code)}', |
| @@ -0,0 +1,349 @@ | |||
| Resolving dependencies... | |||
There was a problem hiding this comment.
This file and the local-artifacts directory appear to be build artifacts or local analyzer outputs. These types of files are generally not committed to source control. Please consider adding local-artifacts/ to your .gitignore file to prevent them from being tracked.
1 participant
Changes
Validation
Notes