-
Notifications
You must be signed in to change notification settings - Fork 0
flutter: add User Assets overview + fix analyzer blockers; wire assets entrances (A/B/C) #64
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
…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
…vigator capture and mounted guards
…rs (accounts/dashboard types, const-eval, family settings void, email Address stub); wire A/B/C entrances
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 adds a "用户资产总览" (User Assets Overview) screen with three navigation entrances (A/B/C) and resolves Flutter analyzer blocking errors to maintain CI health.
- Introduces a new user assets overview screen displaying net worth, total assets/liabilities KPIs, and categorized account lists
- Wires three quick navigation entrances: Dashboard AppBar pie icon (C), Accounts AppBar pie icon (A), and Accounts overflow menu item (B)
- Surgically fixes analyzer blockers including AccountCard constructor issues, const validation errors, void result usage, and missing Address class
Reviewed Changes
Copilot reviewed 46 out of 50 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| jive-flutter/lib/screens/accounts/user_assets_screen.dart | New screen implementing assets overview with net worth display and account categorization |
| jive-flutter/lib/core/router/app_router.dart | Adds userAssets route and nested GoRoute configuration |
| jive-flutter/lib/screens/dashboard/dashboard_screen.dart | Adds pie chart icon entrance (C) to Dashboard AppBar |
| jive-flutter/lib/screens/accounts/accounts_screen.dart | Adds pie chart icon entrance (A) and overflow menu item (B) |
| jive-flutter/lib/ui/components/accounts/account_list.dart | Fixes AccountCard constructor calls to use .fromAccount method |
| jive-flutter/lib/services/email_notification_service.dart | Adds Address stub class for mailer compatibility |
| jive-flutter/lib/services/family_settings_service.dart | Splits void-returning calls to avoid use_of_void_result |
| Multiple widget files | Removes invalid const keywords and fixes build context usage across async gaps |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| borderRadius: BorderRadius.all(Radius.circular(radius)), | ||
| ), | ||
| ), | ||
| inputDecorationTheme: | ||
| Theme.of(context).inputDecorationTheme.copyWith( | ||
| border: OutlineInputBorder( | ||
| borderRadius: BorderRadius.circular(radius), | ||
| borderRadius: BorderRadius.all(Radius.circular(radius)), | ||
| ), | ||
| focusedBorder: OutlineInputBorder( | ||
| borderRadius: BorderRadius.circular(radius), | ||
| borderRadius: BorderRadius.all(Radius.circular(radius)), |
Copilot
AI
Sep 28, 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] Using BorderRadius.circular(radius) is more concise and idiomatic than BorderRadius.all(Radius.circular(radius)).
Copilot uses AI. Check for mistakes.
| borderRadius: BorderRadius.all(Radius.circular(radius)), | ||
| ), | ||
| ), | ||
| inputDecorationTheme: | ||
| Theme.of(context).inputDecorationTheme.copyWith( | ||
| border: OutlineInputBorder( | ||
| borderRadius: BorderRadius.circular(radius), | ||
| borderRadius: BorderRadius.all(Radius.circular(radius)), | ||
| ), | ||
| focusedBorder: OutlineInputBorder( | ||
| borderRadius: BorderRadius.circular(radius), | ||
| borderRadius: BorderRadius.all(Radius.circular(radius)), |
Copilot
AI
Sep 28, 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] Using BorderRadius.circular(radius) is more concise and idiomatic than BorderRadius.all(Radius.circular(radius)).
Copilot uses AI. Check for mistakes.
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 delivers a new 'User Assets Overview' feature to the Flutter application, allowing users to quickly see their financial summary from multiple entry points. Concurrently, it undertakes a significant refactoring effort to resolve a large number of Flutter analyzer warnings and errors, ensuring a cleaner and more robust codebase. The changes also include minor improvements to the Rust backend with added tracing for authentication flows. 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.
Code Review
This pull request successfully adds the new 'User Assets Overview' screen and wires up the entry points as described. A significant portion of the changes also involves fixing a large number of analyzer errors and warnings, which is a great step towards improving code quality and stability. The fixes for use_build_context_synchronously, const correctness, and unused variables are well-executed.
I have a few suggestions to further improve the code, primarily focusing on using more idiomatic Rust for error handling and updating deprecated Flutter API usage in the new user_assets_screen.dart file to prevent future build failures. Overall, this is a solid contribution that adds a useful feature and enhances the codebase's health.
| Expanded( | ||
| child: _buildKpi(theme, '总资产', stats.totalAssets, color: AppConstants.successColor), | ||
| ), | ||
| Container(width: 1, height: 24, color: theme.dividerColor.withValues(alpha: 0.5)), |
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.
The withValues method is an internal detail of the Color class and is being removed from the Flutter SDK. Using it can lead to build failures in future Flutter versions. Please use the public withOpacity method instead to set the color's transparency.
| Container(width: 1, height: 24, color: theme.dividerColor.withValues(alpha: 0.5)), | |
| Container(width: 1, height: 24, color: theme.dividerColor.withOpacity(0.5)), |
| child: Column( | ||
| crossAxisAlignment: CrossAxisAlignment.start, | ||
| children: [ | ||
| Text(label, style: theme.textTheme.bodySmall?.copyWith(color: theme.colorScheme.onSurface.withValues(alpha: 0.6))), |
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.
The withValues method is an internal API and is slated for removal. To ensure future compatibility, please use the recommended withOpacity method to adjust the color's alpha value.
| Text(label, style: theme.textTheme.bodySmall?.copyWith(color: theme.colorScheme.onSurface.withValues(alpha: 0.6))), | |
| Text(label, style: theme.textTheme.bodySmall?.copyWith(color: theme.colorScheme.onSurface.withOpacity(0.6))), |
| .map((a) => ListTile( | ||
| contentPadding: const EdgeInsets.symmetric(horizontal: 0), | ||
| leading: CircleAvatar( | ||
| backgroundColor: (a.displayColor).withValues(alpha: 0.15), |
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.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this match block correctly adds error logging, it's a bit verbose for propagating an error. You can make this more idiomatic and concise by using Result::map_err combined with the ? operator. This achieves the same goal of logging the error before it's propagated.
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
})?;
Purpose
Summary of approach
Testing evidence
Migration/notes
Rollback plan