Read flutterRoot from package_config.json instead of parsing up the flutter package path#5880
Read flutterRoot from package_config.json instead of parsing up the flutter package path#5880
Conversation
…lutter package path Fixes #5728
DanTup
added
is enhancement
|
@codex review |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5880 +/- ##
==========================================
- Coverage 67.32% 67.29% -0.04%
==========================================
Files 168 168
Lines 12894 12899 +5
Branches 2554 2556 +2
==========================================
- Hits 8681 8680 -1
- Misses 3762 3766 +4
- Partials 451 453 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This pull request enhances the Flutter SDK path detection by reading the flutterRoot field directly from package_config.json instead of parsing it from the flutter package path. This provides a more reliable way to determine the Flutter SDK location.
Changes:
- Added
flutterRootPathgetter toPackageMapclass hierarchy to read theflutterRootfield from package config - Updated
extractFlutterSdkPathFromPackagesFileto prioritizeflutterRootfield with fallback to package path parsing - Added comprehensive tests validating both the new direct path reading and the fallback behavior
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/shared/pub/package_map.ts | Added flutterRootPath getter to base PackageMap class and PackageConfigJsonPackageMap, and extended PackageJsonConfig interface to include optional flutterRoot field |
| src/shared/utils/fs.ts | Modified extractFlutterSdkPathFromPackagesFile to prioritize flutterRootPath over package path with nullish coalescing operator |
| src/test/dart/utils/fs.test.ts | Added comprehensive test suite for extractFlutterSdkPathFromPackagesFile covering both package path fallback and flutterRoot prioritization scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
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 "@codex address that feedback". |
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a good improvement, updating the logic to read the Flutter SDK root from package_config.json instead of relying on parsing the flutter package path. This should be more reliable. I've found one potential bug in the implementation where the new flutterRootPath could be incorrectly modified. I've provided a comment with details on the issue. I've also suggested an additional test case to cover this edge case and prevent future regressions. With that fix, this change will be solid.
Fixes #5728