Address PR feedback on FeatureViewModel template#134
Conversation
- Changed [FEATURE_NAME]OverviewViewModel, [FEATURE_NAME]DetailsViewModel, and [FEATURE_NAME]SettingsViewModel from internal to public in the template. - This ensures the scaffolded classes are accessible when the template is used. Co-authored-by: efargas <9705611+efargas@users.noreply.github.com>
- Changed mock ViewModels from internal to public to ensure XAML type resolution compatibility. - Implemented caching for Overview, Details, and Settings ViewModels to preserve state between sidebar navigation. - Added disposal logic for cached ViewModels to prevent resource leaks. - Updated SelectedContentViewModel type to ViewModelBase. Co-authored-by: efargas <9705611+efargas@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Review Summary by QodoImplement ViewModel caching and make mock classes public
WalkthroughsDescription• Implement caching pattern for content ViewModels to preserve state • Change mock ViewModel classes from internal to public • Add proper disposal logic for cached ViewModels • Update SelectedContentViewModel type from object to ViewModelBase Diagramflowchart LR
A["FeatureViewModel Template"] -->|"Caches"| B["OverviewViewModel"]
A -->|"Caches"| C["DetailsViewModel"]
A -->|"Caches"| D["SettingsViewModel"]
B -->|"Implements"| E["ViewModelBase"]
C -->|"Implements"| E
D -->|"Implements"| E
A -->|"Disposes on cleanup"| F["Cached ViewModels"]
File Changes1. docs/templates/ui-integration/FeatureViewModel.template.cs
|
Summary of ChangesHello, 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 addresses feedback on the FeatureViewModel template by enhancing its design for better UI integration and resource management. It makes mock ViewModel types public to support XAML compiled bindings and introduces a caching mechanism for content ViewModels. This ensures that the application's state is maintained across category switches and that resources are correctly released upon ViewModel disposal. Highlights
Changelog
Activity
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
|
Code Review by Qodo
1. Template creates duplicate VMs
|
![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 effectively addresses previous feedback by implementing a caching pattern for the content ViewModels and ensuring their proper disposal. The mock ViewModel types have also been made public to improve compatibility with XAML compiled bindings. The changes improve state management and resource handling. The suggestion regarding code organization for the newly added mock ViewModel classes to align with common C# best practices remains relevant.
efargas
merged commit f0f1adc
into
audit/comprehensive-review-2025
efargas
deleted the
feature/update-viewmodel-template-8714801939707767319
branch
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| #region Mock ViewModels | ||
|
|
||
| /// <summary> | ||
| /// Mock Overview ViewModel for design-time and initial implementation. | ||
| /// </summary> | ||
| public class [FEATURE_NAME]OverviewViewModel : ViewModelBase | ||
| { | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Mock Details ViewModel for design-time and initial implementation. | ||
| /// </summary> | ||
| public class [FEATURE_NAME]DetailsViewModel : ViewModelBase | ||
| { | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Mock Settings ViewModel for design-time and initial implementation. | ||
| /// </summary> | ||
| public class [FEATURE_NAME]SettingsViewModel : ViewModelBase | ||
| { | ||
| } |
There was a problem hiding this comment.
1. Template creates duplicate vms 🐞 Bug ✓ Correctness
The template now defines public stub child ViewModel types ([Feature]Overview/Details/SettingsViewModel) in the same file, while the integration checklist instructs creating those same types as separate files—following the checklist will cause duplicate-type compilation errors. This is a high-likelihood blocker because the README/checklist explicitly tells devs to copy this template into src/ where it will be compiled.
Agent Prompt
## Issue description
The FeatureViewModel template now declares public stub child ViewModel classes (`[Feature]OverviewViewModel`, `[Feature]DetailsViewModel`, `[Feature]SettingsViewModel`) in the same file. The UI integration checklist also instructs developers to create those child ViewModels as separate files, which will produce duplicate-type compile errors when someone follows the documented workflow.
## Issue Context
This template is intended to be copied into `src/` (per README), so its contents will compile as part of the app.
## Fix Focus Areas
- docs/templates/ui-integration/FeatureViewModel.template.cs[264-285]
- docs/templates/ui-integration/INTEGRATION_CHECKLIST.md[34-49]
- docs/templates/ui-integration/README.md[67-82]
## Suggested changes
- Preferred: Change the mock/stub child ViewModel declarations to `public partial class ...` and update the checklist to explicitly instruct creating `partial` implementations in separate files (or extending these partial stubs), avoiding CS0101 duplicate-type errors.
- Alternative: Remove the stub child ViewModels from the main template and add separate templates/files for child ViewModels, updating the checklist/README to match.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
1 participant
I have addressed the Qodo code review suggestions by:
PR created automatically by Jules for task 8714801939707767319 started by @efargas