-
Notifications
You must be signed in to change notification settings - Fork 0
flutter: context cleanup batch 9 — QR share smoke test + dialog context fixes #78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
zensgit
commented
Sep 30, 2025
- Add qr_share_smoke_test (platform-agnostic, uses ShareService._doShare)\n- DeleteFamilyDialog: pre-capture navigator/messenger + mounted guards (previously aligned)\n\nNotes: Remaining analyzer infos (prefer_const_constructors, conservative use_build_context_synchronously) will be handled in small follow-ups. Tests pass locally.
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 enhances the stability and test coverage of the Flutter application. It introduces a new smoke test to validate the QR code sharing mechanism and includes fixes for potential context-related issues within dialogs, ensuring more reliable UI behavior and preventing errors when widgets are unmounted. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances context safety and testing by adding a smoke test for QR sharing functionality and implementing proper context cleanup guards in dialogs. The changes focus on testing the ShareService QR code functionality and improving context handling in dialog operations.
- Add platform-agnostic smoke test for ShareService QR sharing functionality
- Implement pre-capture navigator/messenger pattern with mounted guards in DeleteFamilyDialog
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -0,0 +1,37 @@ | |||
|
|
|||
Copilot
AI
Sep 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Remove the empty line at the beginning of the file to follow standard formatting conventions.
Copilot uses AI. Check for mistakes.
| }); | ||
|
|
||
| await tester.pumpWidget( | ||
| const MaterialApp(home: SizedBox.shrink()), |
Copilot
AI
Sep 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider using Scaffold() instead of SizedBox.shrink() as the home widget. This provides a more realistic widget tree for testing context-dependent operations.
| const MaterialApp(home: SizedBox.shrink()), | |
| const MaterialApp(home: Scaffold()), |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds a smoke test for the QR code sharing functionality. The new test is well-structured. I've suggested an improvement to make the test suite more comprehensive by adding a test case for when the optional description is not provided and by grouping related tests for better organization. This will help ensure the formatting is correct in all scenarios and improve the overall test quality.
| testWidgets('ShareService.shareQrCode shares expected text', (tester) async { | ||
| ShareParams? last; | ||
| ShareService.setDoShareForTest((params) async { | ||
| last = params; | ||
| return const ShareResult('', ShareResultStatus.success); | ||
| }); | ||
|
|
||
| await tester.pumpWidget( | ||
| const MaterialApp(home: SizedBox.shrink()), | ||
| ); | ||
|
|
||
| final ctx = tester.element(find.byType(SizedBox)); | ||
|
|
||
| await ShareService.shareQrCode( | ||
| context: ctx, | ||
| data: 'https://example.com/q/XYZ', | ||
| title: '示例二维码', | ||
| description: '用于测试的二维码', | ||
| ); | ||
|
|
||
| expect(last, isNotNull); | ||
| final text = last!.text ?? ''; | ||
| expect(text, contains('示例二维码')); | ||
| expect(text, contains('用于测试的二维码')); | ||
| expect(text, contains('https://example.com/q/XYZ')); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve test organization and coverage, it's a good practice to group related tests using group(). Additionally, since ShareService.shareQrCode has an optional description parameter, adding a test case for when it's null would make the test suite more robust. This ensures that the text formatting is correct in both scenarios.
There might be a subtle formatting issue in the implementation when description is null (potentially an extra newline). A dedicated test case will help catch and verify this.
group('ShareService.shareQrCode', () {
testWidgets('shares expected text with description', (tester) async {
ShareParams? last;
ShareService.setDoShareForTest((params) async {
last = params;
return const ShareResult('', ShareResultStatus.success);
});
await tester.pumpWidget(
const MaterialApp(home: SizedBox.shrink()),
);
final ctx = tester.element(find.byType(SizedBox));
await ShareService.shareQrCode(
context: ctx,
data: 'https://example.com/q/XYZ',
title: '示例二维码',
description: '用于测试的二维码',
);
expect(last, isNotNull);
final text = last!.text ?? '';
expect(text, contains('示例二维码'));
expect(text, contains('用于测试的二维码'));
expect(text, contains('https://example.com/q/XYZ'));
});
testWidgets('shares expected text without description', (tester) async {
ShareParams? last;
ShareService.setDoShareForTest((params) async {
last = params;
return const ShareResult('', ShareResultStatus.success);
});
await tester.pumpWidget(
const MaterialApp(home: SizedBox.shrink()),
);
final ctx = tester.element(find.byType(SizedBox));
await ShareService.shareQrCode(
context: ctx,
data: 'https://example.com/q/XYZ',
title: '示例二维码',
);
expect(last, isNotNull);
final text = last!.text ?? '';
expect(text, contains('示例二维码'));
expect(text, isNot(contains('用于测试的二维码')));
expect(text, contains('https://example.com/q/XYZ'));
});
});…alog, refreshable_list, currency_converter