Don't fail with invalid paths in the package map#5910
Conversation
If you did `pub get` on Linux and get that package_config into Windows, this would throw and stop the extension from starting up. Fixes #5909
|
@codex review /gemini review |
![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 addresses an issue where invalid paths in package_config.json, often due to cross-platform usage, could cause the extension to crash. The fix involves adding error handling to gracefully manage invalid path URIs by catching exceptions and returning undefined instead of throwing. Additionally, a check is added to ensure that the extracted Flutter SDK path actually exists on the filesystem before it's used. The changes are well-tested with new test cases covering various cross-platform path scenarios. My main feedback is to add logging for the caught exceptions to aid in future debugging.
![chatgpt-codex-connector[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1af040a181
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
There was a problem hiding this comment.
Pull request overview
Prevents the extension from failing to start when a workspace contains a package_config.json generated on a different OS (for example Linux paths opened on Windows), by making package-map URI parsing more resilient and adding coverage for these scenarios.
Changes:
- Catch invalid
file:URI conversions in the package map loader and returnundefinedinstead of throwing. - Make
extractFlutterSdkPathFromPackagesFile()returnundefinedwhen the derived SDK path does not exist. - Expand FS/unit tests to cover cross-OS
file:URI cases for bothflutterRootand the Flutter package entry.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/test/dart/utils/fs.test.ts | Adds tests for cross-OS file: URIs and updates setup to create a fake Flutter root/bin. |
| src/shared/utils/fs.ts | Ensures extracted Flutter SDK path is only returned if it exists on disk. |
| src/shared/pub/package_map.ts | Wraps URI→path conversion in try/catch to avoid crashes on invalid cross-OS URIs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5910 +/- ##
==========================================
- Coverage 67.65% 67.61% -0.04%
==========================================
Files 169 169
Lines 12976 12979 +3
Branches 2564 2566 +2
==========================================
- Hits 8779 8776 -3
- Misses 3743 3748 +5
- Partials 454 455 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
DanTup
force-pushed
the
handle-bad-package-map
branch
from
da7e445 to
eaa8a85
Compare
DanTup
force-pushed
the
handle-bad-package-map
branch
from
eaa8a85 to
bbdaafe
Compare
If you did
pub geton Linux and get that package_config into Windows, this would throw and stop the extension from starting up.Fixes #5909