feat(banks): minimal Bank Selector — API + Flutter component #68
Conversation
… component and embed in AccountAddScreen (optional)
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 introduces the foundational infrastructure for managing and selecting banks and financial institutions within the application. It encompasses database schema changes, a new API endpoint for data retrieval, and a comprehensive Flutter component for user interaction. This work establishes a robust framework for future enhancements related to bank integration and account management. 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 implements a minimal bank selector feature with API backend support and Flutter UI components. It enables users to search and select banks or cryptocurrencies when adding accounts.
- Added database structure for banks with search capabilities (pinyin support)
- Implemented REST API endpoint for fetching banks with filtering options
- Created Flutter components for bank selection with caching and search functionality
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| jive-api/migrations/031_create_banks_table.sql | Database table creation for banks with indexes for search optimization |
| jive-api/src/models/bank.rs | Bank model struct with display name helper method |
| jive-api/src/handlers/banks.rs | API handler for listing banks with search and crypto filtering |
| jive-flutter/lib/models/bank.dart | Flutter Bank model with JSON serialization and display properties |
| jive-flutter/lib/services/bank_service.dart | Service layer with caching, API calls, and local filtering |
| jive-flutter/lib/ui/components/banks/bank_selector.dart | UI component for bank selection with search modal |
| jive-flutter/lib/screens/accounts/account_add_screen.dart | Minor updates to variable naming and comments |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| child: Text( | ||
| widget.selectedBank!.displayName[0].toUpperCase(), | ||
| style: theme.textTheme.bodySmall, | ||
| ), | ||
| ) | ||
| else | ||
| Icon( | ||
| widget.isCryptoMode | ||
| ? Icons.currency_bitcoin | ||
| : Icons.account_balance, | ||
| size: 20, |
There was a problem hiding this comment.
The condition checks if iconFilename is not null but shows a text avatar instead of the icon. The logic should be reversed - show the text avatar when iconFilename is null.
| child: Text( | |
| widget.selectedBank!.displayName[0].toUpperCase(), | |
| style: theme.textTheme.bodySmall, | |
| ), | |
| ) | |
| else | |
| Icon( | |
| widget.isCryptoMode | |
| ? Icons.currency_bitcoin | |
| : Icons.account_balance, | |
| size: 20, | |
| backgroundImage: AssetImage(widget.selectedBank!.iconFilename!), | |
| ) | |
| else | |
| CircleAvatar( | |
| radius: 12, | |
| backgroundColor: theme.colorScheme.surfaceContainerHighest, | |
| child: Text( | |
| widget.selectedBank!.displayName[0].toUpperCase(), | |
| style: theme.textTheme.bodySmall, | |
| ), |
Copilot uses AI. Check for mistakes.
| child: Text( | ||
| bank.displayName[0].toUpperCase(), | ||
| style: theme.textTheme.titleMedium, | ||
| ), | ||
| ) | ||
| : Icon( | ||
| widget.isCryptoMode | ||
| ? Icons.currency_bitcoin | ||
| : Icons.account_balance, | ||
| ), |
There was a problem hiding this comment.
Same logic error as above - when iconFilename is not null, it shows a text avatar instead of using the actual icon. The condition should be reversed.
| child: Text( | |
| bank.displayName[0].toUpperCase(), | |
| style: theme.textTheme.titleMedium, | |
| ), | |
| ) | |
| : Icon( | |
| widget.isCryptoMode | |
| ? Icons.currency_bitcoin | |
| : Icons.account_balance, | |
| ), | |
| backgroundImage: AssetImage(bank.iconFilename!), | |
| ) | |
| : (widget.isCryptoMode | |
| ? Icon(Icons.currency_bitcoin) | |
| : CircleAvatar( | |
| backgroundColor: theme.colorScheme.surfaceContainerHighest, | |
| child: Text( | |
| bank.displayName[0].toUpperCase(), | |
| style: theme.textTheme.titleMedium, | |
| ), | |
| )), |
Copilot uses AI. Check for mistakes.
| try { | ||
| return banks.firstWhere((bank) => bank.id == id); | ||
| } catch (e) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Using firstWhere without orElse will throw a StateError if no bank is found. The catch block won't handle this exception type properly. Use firstWhere with orElse parameter or use where().firstOrNull if available.
| try { | |
| return banks.firstWhere((bank) => bank.id == id); | |
| } catch (e) { | |
| return null; | |
| } | |
| return banks.firstWhere( | |
| (bank) => bank.id == id, | |
| orElse: () => null, | |
| ); |
Copilot uses AI. Check for mistakes.
| borderRadius: | ||
| BorderRadius.circular(AppConstants.borderRadius), | ||
| ), | ||
| suffixIcon: _searchController.text.isNotEmpty |
There was a problem hiding this comment.
This condition won't update when the text changes because it's not wrapped in a StatefulBuilder or listening to controller changes. The clear button won't appear/disappear as expected during typing.
Copilot uses AI. Check for mistakes.
![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 introduces a new bank selector feature, complete with a database migration, a new API endpoint, and a Flutter component. The implementation is a solid first step. My review focuses on several areas for improvement: ensuring consistency between client and server search logic, optimizing data fetching and caching strategies, improving code quality and maintainability in the Rust backend, and fixing a UI bug where bank icons are not displayed. Addressing these points will enhance the feature's correctness, performance, and user experience.
| let banks = query | ||
| .build() | ||
| .fetch_all(&pool) | ||
| .await | ||
| .map_err(|e| ApiError::DatabaseError(e.to_string()))?; | ||
|
|
||
| let mut response = Vec::new(); | ||
| for row in banks { | ||
| response.push(Bank { | ||
| id: row.get("id"), | ||
| code: row.get("code"), | ||
| name: row.get("name"), | ||
| name_cn: row.get("name_cn"), | ||
| name_en: row.get("name_en"), | ||
| icon_filename: row.get("icon_filename"), | ||
| is_crypto: row.get("is_crypto"), | ||
| }); | ||
| } | ||
|
|
||
| Ok(Json(response)) |
There was a problem hiding this comment.
The current implementation manually maps each PgRow to the Bank struct. This is verbose and error-prone, as any change in the SELECT statement or Bank struct requires manual updates in two places.
You can use sqlx's automatic mapping capabilities to make the code more concise, readable, and safer. To do this, you'll need to:
- Add
#[derive(sqlx::FromRow)]to yourBankstruct definition injive-api/src/models/bank.rs. - Use
build_query_as()and letsqlxhandle the mapping.
This change significantly improves maintainability and reduces the chance of runtime errors from mismatched columns.
let banks: Vec<Bank> = query
.build_query_as()
.fetch_all(&pool)
.await
.map_err(|e| ApiError::DatabaseError(e.to_string()))?;
Ok(Json(banks))| if (!forceRefresh) { | ||
| final cached = await _getCachedBanks(); | ||
| if (cached != null && cached.isNotEmpty) { | ||
| return _filterBanksLocally(cached, search: search, isCrypto: isCrypto); | ||
| } | ||
| } |
There was a problem hiding this comment.
The caching logic in getBanks is flawed. When a search term is provided, it filters the locally cached list instead of querying the API. This leads to incorrect and incomplete search results because the client-side filtering is less capable than the backend's (e.g., no pinyin search).
The API should always be queried when a search parameter is present to ensure accurate results. The cache should only be used for requests that fetch the entire, unfiltered list.
// Only use cache if not searching and not forcing a refresh.
if (!forceRefresh && (search == null || search.isEmpty)) {
final cached = await _getCachedBanks();
if (cached != null && cached.isNotEmpty) {
// Apply isCrypto filter locally if needed on the cached full list.
return _filterBanksLocally(cached, isCrypto: isCrypto);
}
}| void _filterBanks() { | ||
| final query = _searchController.text.toLowerCase(); | ||
| setState(() { | ||
| if (query.isEmpty) { | ||
| _filteredBanks = widget.banks; | ||
| } else { | ||
| _filteredBanks = widget.banks.where((bank) { | ||
| return bank.name.toLowerCase().contains(query) || | ||
| (bank.nameCn?.toLowerCase().contains(query) ?? false) || | ||
| (bank.nameEn?.toLowerCase().contains(query) ?? false) || | ||
| bank.code.toLowerCase().contains(query); | ||
| }).toList(); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
The search functionality in the _BankPickerSheet is implemented purely on the client-side, filtering a pre-loaded list of banks. This is inconsistent with the backend's more powerful search capabilities (which include pinyin and abbreviations) and can lead to a poor user experience.
A better approach is to perform the search on the server. This typically involves:
- Debouncing the user's input in the search
TextFieldto avoid excessive API calls. - Calling the
BankServiceto fetch results from the API with the search query (e.g.,_bankService.getBanks(search: query)). - Updating the UI with the results from the API, including loading and error states for the search operation itself.
This will provide a more consistent, accurate, and scalable search experience.
| name_cn_pinyin VARCHAR(200), | ||
| name_cn_abbr VARCHAR(50), | ||
| icon_filename VARCHAR(100), | ||
| icon_url TEXT, |
There was a problem hiding this comment.
The icon_url column is defined in the banks table schema but appears to be unused. The API handler in jive-api/src/handlers/banks.rs does not select this column, and the Flutter client in jive-flutter/lib/models/bank.dart dynamically constructs the icon URL from icon_filename. To avoid confusion and redundant data storage, this column should be removed.
| updated_at TIMESTAMP DEFAULT NOW() | ||
| ); | ||
|
|
||
| CREATE INDEX idx_banks_code ON banks(code); |
There was a problem hiding this comment.
The index idx_banks_code on the code column is redundant. PostgreSQL automatically creates an index for columns with a UNIQUE constraint, which the code column has. Removing this explicit CREATE INDEX statement will simplify the migration script without affecting performance.
| if let Some(search) = params.search { | ||
| query.push(" AND ("); | ||
| query.push("name_cn ILIKE "); | ||
| query.push_bind(format!("%{}%", search)); | ||
| query.push(" OR name ILIKE "); | ||
| query.push_bind(format!("%{}%", search)); | ||
| query.push(" OR name_en ILIKE "); | ||
| query.push_bind(format!("%{}%", search)); | ||
| query.push(" OR name_cn_pinyin ILIKE "); | ||
| query.push_bind(format!("%{}%", search)); | ||
| query.push(" OR name_cn_abbr ILIKE "); | ||
| query.push_bind(format!("%{}%", search)); | ||
| query.push(")"); | ||
| } |
There was a problem hiding this comment.
The logic for building the search query involves repeating query.push(...) and query.push_bind(...) for each searchable field. This can be made more maintainable and less repetitive by iterating over a list of column names.
if let Some(search) = params.search {
let search_pattern = format!("%{}%", search);
let search_fields = ["name_cn", "name", "name_en", "name_cn_pinyin", "name_cn_abbr"];
query.push(" AND (");
for (i, field) in search_fields.iter().enumerate() {
if i > 0 {
query.push(" OR ");
}
query.push(format!("{} ILIKE ", field));
query.push_bind(search_pattern.clone());
}
query.push(")");
}| String? get iconUrl => iconFilename != null | ||
| ? '/static/bank_icons/$iconFilename' | ||
| : null; |
There was a problem hiding this comment.
The iconUrl getter constructs a relative URL. When using this with Image.network, you'll need to prepend the API's base URL. To make this model more self-contained and easier to use across the app, consider making the base URL available here, perhaps through a static configuration class, and returning a full URL.
| Future<Bank?> getBankById(String id) async { | ||
| final banks = await getBanks(); | ||
| try { | ||
| return banks.firstWhere((bank) => bank.id == id); | ||
| } catch (e) { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
The getBankById method currently fetches the entire list of banks and then finds the one with the matching ID on the client side. This is inefficient, especially as the number of banks grows, as it leads to unnecessary data transfer and processing.
For better performance, consider adding a dedicated API endpoint to fetch a single bank by its ID, such as GET /api/v1/banks/:id. This would make the lookup much more efficient.
| if (widget.selectedBank!.iconFilename != null) | ||
| CircleAvatar( | ||
| radius: 12, | ||
| backgroundColor: theme.colorScheme.surfaceContainerHighest, | ||
| child: Text( | ||
| widget.selectedBank!.displayName[0].toUpperCase(), | ||
| style: theme.textTheme.bodySmall, | ||
| ), | ||
| ) | ||
| else | ||
| Icon( | ||
| widget.isCryptoMode | ||
| ? Icons.currency_bitcoin | ||
| : Icons.account_balance, | ||
| size: 20, | ||
| ), |
There was a problem hiding this comment.
The bank icon is not being displayed. The code checks for iconFilename but then displays a CircleAvatar with the first letter of the bank's name instead of the actual icon image. This happens for both the selected bank display and the items in the picker list.
To fix this, you should use an Image.network widget to display the icon. You'll need to construct the full URL using your API's base URL (e.g., from ApiConfig).
Example for the selected bank:
if (widget.selectedBank!.iconUrl != null) {
Image.network(
ApiConfig.apiUrl + widget.selectedBank!.iconUrl!,
width: 24,
height: 24,
errorBuilder: (context, error, stackTrace) => Icon(
widget.isCryptoMode ? Icons.currency_bitcoin : Icons.account_balance,
size: 20,
),
);
} else {
// ... fallback icon
}A similar change is needed in _BankPickerSheet for the ListTile's leading widget.
- Remove REFERENCES payees(id) constraint (payees table doesn't exist yet) - Add TODO comment for future FK constraint - Comment out account module (not in this branch) - Fixes CI compilation error: relation payees does not exist 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
… transaction_provider - Resolved conflict in family_settings_service.dart by keeping main's parameterized method calls - Resolved conflict in transaction_provider.dart by removing duplicate enum and method definitions - Both files now compile without errors 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Resolved git merge conflicts that were polluting all feature branches: - theme_management_screen.dart: 5 conflicts (ScaffoldMessenger patterns) - transaction_provider.dart: 2 conflicts (duplicate enum & method definitions) - family_activity_log_screen.dart: 1 conflict (statistics loading) All conflicts resolved by: - Preferring messenger variable pattern over repeated ScaffoldMessenger.of() - Removing duplicate enum and method definitions - Using direct stats return instead of _parseActivityStatistics() Added comprehensive PR fix report documenting all 5 PRs analyzed. This fix will automatically benefit all PRs (#65, #66, #68, #69, #70) as they rebase/merge from main. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Resolved conflicts in transaction_provider.dart by accepting main's clean version. Main branch already contains fixes for duplicate enums and methods.
Merged after resolving conflicts with updated main (which includes PR #68)
Resolve merge conflicts: - jive-api/src/main.rs: Keep bank icons static route at end - jive-flutter/lib/services/family_settings_service.dart: Remove extra blank line 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
Adds banks table migration, list endpoint (GET /api/v1/banks), Flutter BankSelector with search and optional embedding in AccountAddScreen. No category/template changes included.\n\n- API: jive-api/src/handlers/banks.rs, route wired in main.rs\n- Flutter: models/bank.dart, services/bank_service.dart, ui/components/banks/bank_selector.dart\n- Screen: AccountAddScreen optional BankSelector field (no payload change yet)\n\nFollow-ups: bank version endpoint, static bank_icons route, wire bank_id on save.