-
Notifications
You must be signed in to change notification settings - Fork 84
fix(autocomplete): fix a semver check in old autocomplete #2499
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
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 fixes a bug in the old autocomplete logic by properly handling semver ranges in version checks.
- Introduces a
satisfiesVersionhelper to usesemver.satisfiesfor both exact versions and ranges. - Replaces the direct
semver.gtecall inisAcceptablewithsatisfiesVersion.
Comments suppressed due to low confidence (1)
packages/autocomplete/src/index.ts:319
- Add unit tests for
satisfiesVersionto cover both plain version inputs and complex range expressions to ensure future changes don’t regress this logic.
satisfiesVersion(
| const isGTECheck = /^\d+?\.\d+?\.\d+?$/.test(v2); | ||
| return semver.satisfies(v1, isGTECheck ? `>=${v2}` : v2); |
Copilot
AI
Jul 7, 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.
Rather than a custom regex, consider using semver.valid(v2) to detect exact version strings, which will handle pre-release and build metadata more robustly.
| const isGTECheck = /^\d+?\.\d+?\.\d+?$/.test(v2); | |
| return semver.satisfies(v1, isGTECheck ? `>=${v2}` : v2); | |
| const isValidVersion = semver.valid(v2); | |
| return isValidVersion && semver.satisfies(v1, isValidVersion.startsWith('>=') ? v2 : `>=${v2}`); |
| 'db.shipwrecks.aggregate([{$sortByCount', | ||
| 'db.shipwrecks.aggregate([{$unionWith', | ||
| 'db.shipwrecks.aggregate([{$unset', | ||
| 'db.shipwrecks.aggregate([{$unwind', |
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.
Did this test error before? I think we could just verify that it doesn't throw, rather than asserting against the full list, since this list probably changes over time (while the old autocompleter exists, at least)
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.
on server 4.4, though? I suppose I can change it.
I was comparing old and new autocomplete when I noticed this bug in old autocomplete.
type:
and you get this error: