Category: restore management page import flow (dry-run preview + details bottom sheet) #14
Conversation
…iew; show details bottom sheet after import
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 reintroduces the enhanced category management page, previously a placeholder, by implementing a streamlined template import feature. Users can now access a template library, select categories, and perform a dry-run preview to understand the impact of their import choices. Upon confirmation, a detailed summary of the import process is displayed, ensuring transparency and control over category management. The current scope is intentionally focused on this core import functionality, laying the groundwork for future enhancements. 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
Restores the CategoryManagementEnhancedPage with a minimal template import dialog that includes dry-run preview functionality and an import details bottom sheet.
- Converts placeholder page to functional widget with template import flow
- Implements dry-run preview with server-side validation before actual import
- Adds ImportDetailsSheet to display grouped import results with JSON export capability
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| DropdownMenuItem(value: 'rename', child: Text('重命名')), | ||
| DropdownMenuItem(value: 'update', child: Text('覆盖')), | ||
| ], | ||
| onChanged: (v) { if (v!=null) setLocal((){ conflict = v; }); }, |
There was a problem hiding this comment.
Missing space after 'if' and around '!=' operator. Should be formatted as 'if (v != null)' for consistency with Dart style guidelines.
| onChanged: (v) { if (v!=null) setLocal((){ conflict = v; }); }, | |
| onChanged: (v) { if (v != null) setLocal((){ conflict = v; }); }, |
Copilot uses AI. Check for mistakes.
| onChanged: (_) => setLocal((){ | ||
| if (checked) { selected.remove(t); } else { selected.add(t); } |
There was a problem hiding this comment.
Missing space after 'setLocal' before the opening parenthesis. Should be formatted as 'setLocal(() {' for consistency with Dart formatting standards.
Copilot uses AI. Check for mistakes.
| onConflict: conflict, | ||
| dryRun: true, | ||
| ); | ||
| setLocal((){ preview = res; }); |
There was a problem hiding this comment.
Missing space after opening parenthesis. Should be formatted as 'setLocal(() { preview = res; });' for consistency with Dart formatting standards.
| setLocal((){ preview = res; }); | |
| setLocal(() { preview = res; }); |
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 successfully restores the category management page by introducing a template import feature. The implementation is well-structured, featuring a dialog for template selection, a dry-run preview, and a detailed import result sheet. My review includes suggestions to enhance error handling, improve user experience by adding a missing loading indicator, and increase code maintainability by addressing duplicated logic and hardcoded values. Overall, this is a solid contribution that brings back important functionality.
| try { | ||
| templates = await CategoryServiceIntegrated().getAllTemplates(forceRefresh: true); | ||
| } catch (_) {} |
There was a problem hiding this comment.
This try-catch block currently swallows errors silently. If fetching templates fails, the user will be presented with an empty list in the dialog without any explanation, which can be confusing. It's important to provide feedback to the user when an error occurs, for instance, by showing a SnackBar.
| try { | |
| templates = await CategoryServiceIntegrated().getAllTemplates(forceRefresh: true); | |
| } catch (_) {} | |
| try { | |
| templates = await CategoryServiceIntegrated().getAllTemplates(forceRefresh: true); | |
| } catch (e) { | |
| if (mounted) { | |
| ScaffoldMessenger.of(context).showSnackBar(SnackBar(content: Text('获取模板失败: $e'))); | |
| } | |
| } |
| try { | ||
| templates = await CategoryServiceIntegrated().getAllTemplates(forceRefresh: true); | ||
| } catch (_) {} | ||
| if (!mounted) return; |
There was a problem hiding this comment.
Using if (!mounted) after an await is a common way to prevent errors from calling setState on a disposed widget, but it's often considered an anti-pattern in modern Flutter development. It can indicate that state updates are not properly tied to the widget's lifecycle.
For more robust asynchronous operations, consider using a FutureProvider or StateNotifier from Riverpod to manage the loading state and data fetching. This would handle the widget lifecycle automatically and remove the need for manual mounted checks and setState calls for the _busy flag.
| itemCount: preview!.details.length, | ||
| itemBuilder: (_, i) { | ||
| final d = preview!.details[i]; | ||
| final color = (d.action == 'failed' || d.action == 'skipped') ? Colors.orange : Colors.green; |
There was a problem hiding this comment.
Hardcoded colors like Colors.orange and Colors.green are used here to indicate status. It's a better practice to use colors from the app's theme (e.g., Theme.of(context).colorScheme) to ensure UI consistency and support for different themes (like light and dark mode). For example, you could use Theme.of(context).colorScheme.primary for success and Theme.of(context).colorScheme.error for failures.
| actions: [ | ||
| TextButton(onPressed: () => Navigator.pop(ctx), child: const Text('取消')), | ||
| TextButton( | ||
| onPressed: selected.isEmpty ? null : () async { | ||
| try { | ||
| final items = selected.map((t) => { 'template_id': t.id }).toList(); | ||
| final res = await CategoryService().importTemplatesAdvanced( | ||
| ledgerId: ledgerId, | ||
| items: items, | ||
| onConflict: conflict, | ||
| dryRun: true, | ||
| ); | ||
| setLocal((){ preview = res; }); | ||
| } catch (e) { | ||
| if (context.mounted) { | ||
| ScaffoldMessenger.of(context).showSnackBar(SnackBar(content: Text('预览失败: $e'))); | ||
| } | ||
| } | ||
| }, | ||
| child: const Text('预览'), | ||
| ), | ||
| FilledButton( | ||
| onPressed: (selected.isEmpty) ? null : () async { | ||
| Navigator.pop(ctx); | ||
| try { | ||
| final items = selected.map((t) => { 'template_id': t.id }).toList(); | ||
| final result = await CategoryService().importTemplatesAdvanced( | ||
| ledgerId: ledgerId, | ||
| items: items, | ||
| onConflict: conflict, | ||
| ); | ||
| if (!mounted) return; | ||
| await ref.read(userCategoriesProvider.notifier).refreshFromBackend(ledgerId: ledgerId); | ||
| await ImportDetailsSheet.show(context, result); | ||
| } catch (e) { | ||
| if (!mounted) return; | ||
| ScaffoldMessenger.of(context).showSnackBar(SnackBar(content: Text('导入失败: $e'))); | ||
| } | ||
| }, | ||
| child: const Text('确认导入'), | ||
| ), | ||
| ], |
There was a problem hiding this comment.
The logic to create the items list from the selected templates is duplicated in the onPressed handlers for both the '预览' (Preview) and '确认导入' (Confirm Import) buttons (lines 142 and 162).
To improve maintainability and avoid redundancy, you can extract this logic. A good approach would be to define final items = selected.map((t) => { 'template_id': t.id }).toList(); at the beginning of the StatefulBuilder's builder method and then reuse the items variable in both handlers.
| onPressed: (selected.isEmpty) ? null : () async { | ||
| Navigator.pop(ctx); | ||
| try { | ||
| final items = selected.map((t) => { 'template_id': t.id }).toList(); | ||
| final result = await CategoryService().importTemplatesAdvanced( | ||
| ledgerId: ledgerId, | ||
| items: items, | ||
| onConflict: conflict, | ||
| ); | ||
| if (!mounted) return; | ||
| await ref.read(userCategoriesProvider.notifier).refreshFromBackend(ledgerId: ledgerId); | ||
| await ImportDetailsSheet.show(context, result); | ||
| } catch (e) { | ||
| if (!mounted) return; | ||
| ScaffoldMessenger.of(context).showSnackBar(SnackBar(content: Text('导入失败: $e'))); | ||
| } | ||
| }, |
There was a problem hiding this comment.
When the '确认导入' (Confirm Import) button is pressed, the dialog is closed, and the import process begins. However, the main screen doesn't show a loading indicator because the _busy state is not updated. This can make the UI feel unresponsive during the import. It's better to provide visual feedback to the user that an operation is in progress.
onPressed: (selected.isEmpty) ? null : () async {
Navigator.pop(ctx);
setState(() { _busy = true; });
try {
final items = selected.map((t) => { 'template_id': t.id }).toList();
final result = await CategoryService().importTemplatesAdvanced(
ledgerId: ledgerId,
items: items,
onConflict: conflict,
);
if (!mounted) return;
await ref.read(userCategoriesProvider.notifier).refreshFromBackend(ledgerId: ledgerId);
await ImportDetailsSheet.show(context, result);
} catch (e) {
if (!mounted) return;
ScaffoldMessenger.of(context).showSnackBar(SnackBar(content: Text('导入失败: $e')));
} finally {
if (mounted) {
setState(() { _busy = false; });
}
}
},1 participant
Restores CategoryManagementEnhancedPage with a minimal template import dialog.\n- Dry-run preview calls backend with dry_run=true and renders details\n- After import, shows ImportDetailsSheet with grouped details and copy JSON\n- Keeps scope minimal; no advanced ops included