-
Notifications
You must be signed in to change notification settings - Fork 340
Improve batch execution of commands #5746
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
|
@codex review |
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 improves batch execution of commands (pub get, pub upgrade, flutter clean) by consolidating multiple individual notifications into a single progress notification. The key enhancement is that commands now report the project names as they process each item, and cancellation properly works across the entire batch operation.
- Adds a unified progress reporting mechanism through the
OperationProgressinterface - Implements batch-aware command execution that shows project names in progress messages
- Ensures cancellation tokens work correctly across batch operations
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/extension/commands/sdk.ts | Adds OperationProgress interface and updates command methods to support optional progress reporting |
| src/extension/commands/packages.ts | Refactors pub get/upgrade commands to use unified progress reporting for batch operations |
| src/extension/commands/flutter.ts | Updates flutter clean command to support batch progress reporting and fixes parameter order issue |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (!fs.existsSync(tempDir)) | ||
| fs.mkdirSync(tempDir); | ||
| return this.runFlutterInFolder(tempDir, ["doctor", "-v"], "flutter", true, this.workspace.config?.flutterDoctorScript); | ||
| return this.runFlutterInFolder(tempDir, ["doctor", "-v"], "flutter", true, undefined, this.workspace.config?.flutterDoctorScript); |
Copilot
AI
Oct 9, 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.
The parameter order has changed but this call passes undefined for the new operationProgress parameter and then the customScript parameter. This suggests the method signature may have been updated incorrectly. Verify that the parameter order matches the method definition.
|
Thank you for requesting a code review. I am now performing a comprehensive review of the pull request, focusing on correctness, efficiency, maintainability, and security, as per the guidelines. I will provide my feedback shortly. |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting |
Improves "pub get", "pub upgrade" and "flutter clean" when run in-batch. We now produce a single notification and report the names of the projects as we go through them. This also means that cancellation works correctly.
Fixes #5745
Fixes #5740