EDM-3750: Validate catalogItem versions correctly#626
Conversation
WalkthroughThis PR enforces stricter semantic version validation by disallowing leading "v" prefixes in version strings and skip ranges, renames enum members in ChangesSemver Validation & Type Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
celdrake
force-pushed
the
EDM-3750-catalog-items-version-validations
branch
from
4634b6b to
cf0a289
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/types/models/K8sProviderSpec.ts (1)
1-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis generated file must not be hand-edited — regenerate via
npm run gen-types.The file header (
/* generated using openapi-typescript-codegen -- do no edit */) and the project's coding guidelines both explicitly prohibit manual modifications to files underlibs/types/. The nullable changes toorganizationAssignmentandroleAssignmentappear to be made by hand here, while the upstream OpenAPI schema (from flightctl PR#2687) already reflects this change. Runningnpm run gen-typesfrom the repo root should produce the same output without violating the "no hand-edit" rule, and will also keep any other generated artifacts in sync.As per coding guidelines: "Models and API types in
libs/typesare generated from the flightctl OpenAPI schema and must not be modified by hand. Regenerate usingnpm run gen-typesfrom the repo root."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/types/models/K8sProviderSpec.ts` around lines 1 - 43, Revert the manual edits in the generated K8sProviderSpec type (specifically the nullable changes to the properties organizationAssignment and roleAssignment) and do not hand-edit files under libs/types; instead regenerate the types from the authoritative OpenAPI schema by running the generator from the repo root (npm run gen-types) so the K8sProviderSpec (and any other generated models) are updated correctly and consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/ui-components/src/components/Catalog/AddCatalogItemWizard/utils.ts`:
- Around line 358-375: The validator is checking for a leading "v" after calling
semver.validRange(), so inputs like "v1.0.0" slip through; update
isValidCatalogSemverRange to first inspect the original trimmed input for
forbidden leading "v" on each token (use RANGE_OPERATOR_TRIM to strip
operators/spaces and hasForbiddenLeadingVPrefix to detect the prefix) and return
false if any token has a leading "v" before calling semver.validRange(), then
proceed with normalization and the existing checks; also add unit tests for
"v1.0.0", ">=v1.0.0 <2.0.0", ">= v1.0.0 < 2.0.0", and "v1.0.0 - v2.0.0" to cover
these cases.
---
Outside diff comments:
In `@libs/types/models/K8sProviderSpec.ts`:
- Around line 1-43: Revert the manual edits in the generated K8sProviderSpec
type (specifically the nullable changes to the properties organizationAssignment
and roleAssignment) and do not hand-edit files under libs/types; instead
regenerate the types from the authoritative OpenAPI schema by running the
generator from the repo root (npm run gen-types) so the K8sProviderSpec (and any
other generated models) are updated correctly and consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e18ff569-7a69-4171-8136-e1bf7b258222
📒 Files selected for processing (6)
libs/i18n/locales/en/translation.jsonlibs/types/alpha/models/ApiVersion.tslibs/types/alpha/models/CatalogItemVersion.tslibs/types/models/K8sProviderSpec.tslibs/ui-components/src/components/Catalog/AddCatalogItemWizard/utils.tslibs/ui-components/src/constants.ts
| const isValidCatalogSemverRange = (value: string): boolean => { | ||
| const trimmed = value.trim(); | ||
| if (!trimmed) { | ||
| return false; | ||
| } | ||
| const normalizedRange = semver.validRange(trimmed); | ||
| if (normalizedRange === null) { | ||
| return false; | ||
| } | ||
| const versionParts = normalizedRange.split(/\s+/); | ||
| for (const part of versionParts) { | ||
| const version = part.replace(RANGE_OPERATOR_TRIM, ''); | ||
| if (!version || hasForbiddenLeadingVPrefix(version)) { | ||
| return false; | ||
| } | ||
| return semver.valid(value) !== null; | ||
| }); | ||
| } | ||
| return true; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm semver.validRange strips leading "v" in 7.7.3 and that current code accepts these inputs.
node -e '
const semver = require("semver");
const cases = ["v1.0.0", ">=v1.0.0 <2.0.0", ">= v1.0.0 < 2.0.0", "v1.0.0 - v2.0.0"];
for (const c of cases) console.log(JSON.stringify(c), "->", JSON.stringify(semver.validRange(c)));
'Repository: flightctl/flightctl-ui
Length of output: 203
🏁 Script executed:
cat -n libs/ui-components/src/components/Catalog/AddCatalogItemWizard/utils.ts | sed -n '340,385p'Repository: flightctl/flightctl-ui
Length of output: 1807
🏁 Script executed:
rg -n "hasForbiddenLeadingVPrefix" libs/ui-components/src/components/Catalog/AddCatalogItemWizard/utils.ts -A 3 -B 1Repository: flightctl/flightctl-ui
Length of output: 537
🏁 Script executed:
fd -e "\.test\.(ts|tsx|js|jsx)$" libs/ui-components/src/components/Catalog/AddCatalogItemWizard | head -5Repository: flightctl/flightctl-ui
Length of output: 620
🏁 Script executed:
fd . 'libs/ui-components/src/components/Catalog/AddCatalogItemWizard' --type fRepository: flightctl/flightctl-ui
Length of output: 812
🏁 Script executed:
rg -l "isValidCatalogSemverRange|optionalSemverRange" libs/ui-components --type ts --type tsxRepository: flightctl/flightctl-ui
Length of output: 94
🏁 Script executed:
rg -l "isValidCatalogSemverRange|optionalSemverRange" libs/ui-components -t tsRepository: flightctl/flightctl-ui
Length of output: 138
🏁 Script executed:
fd . 'libs/ui-components/src/components/Catalog/AddCatalogItemWizard' --type f -name "*.test.*"Repository: flightctl/flightctl-ui
Length of output: 297
🏁 Script executed:
rg "optionalSemverRange|isValidCatalogSemverRange" libs/ui-components/src/components/Catalog/AddCatalogItemWizard/utils.ts -A 5 -B 2Repository: flightctl/flightctl-ui
Length of output: 1236
🏁 Script executed:
fd . 'libs/ui-components/src/components/Catalog/AddCatalogItemWizard' --type f | grep -i testRepository: flightctl/flightctl-ui
Length of output: 48
🏁 Script executed:
rg "v1\.0\.0|hasForbiddenLeadingVPrefix" libs/ui-components --type ts -lRepository: flightctl/flightctl-ui
Length of output: 138
🏁 Script executed:
cat -n libs/ui-components/src/components/Catalog/AddCatalogItemWizard/utils.ts | sed -n '386,430p'Repository: flightctl/flightctl-ui
Length of output: 1623
🏁 Script executed:
rg "isValidCatalogSemverRange|isValidCatalogSingleSemver|optionalSemver" libs/ui-components/src/components/Catalog/AddCatalogItemWizard -A 10 | grep -i "test\|describe\|it("Repository: flightctl/flightctl-ui
Length of output: 979
🏁 Script executed:
find libs/ui-components -name "*.test.ts" -o -name "*.test.tsx" -o -name "*.spec.ts" -o -name "*.spec.tsx" 2>/dev/null | head -10Repository: flightctl/flightctl-ui
Length of output: 48
🏁 Script executed:
cat -n libs/ui-components/src/components/Catalog/AddCatalogItemWizard/utils.ts | sed -n '1,50p'Repository: flightctl/flightctl-ui
Length of output: 1833
Range validator accepts leading "v" because it checks the normalized output instead of the user input.
semver.validRange() strips a leading v before returning the normalized range (per semver 7.7.3 docs). The current isValidCatalogSemverRange implementation normalizes first (line 363) and then checks for a leading v on the normalized parts (lines 367–373), meaning the check can never catch user input like "v1.0.0", ">=v1.0.0 <2.0.0", or "v1.0.0 - v2.0.0". All of these pass validation and are silently normalized to clean output by toApiCatalogSkipRange (line 383), defeating the error message on line 418 that says "Leading "v" is not allowed."
By contrast, isValidCatalogSingleSemver (lines 350–351) correctly checks the original string first before calling semver.valid().
Move the leading-v check to the original input before delegating to semver.validRange():
Proposed fix
const isValidCatalogSemverRange = (value: string): boolean => {
const trimmed = value.trim();
if (!trimmed) {
return false;
}
+ // Check for leading "v" in the original input *before* semver strips it away.
+ for (const part of trimmed.split(/\s+/)) {
+ const operand = part.replace(RANGE_OPERATOR_TRIM, '');
+ if (hasForbiddenLeadingVPrefix(operand)) {
+ return false;
+ }
+ }
const normalizedRange = semver.validRange(trimmed);
if (normalizedRange === null) {
return false;
}
- const versionParts = normalizedRange.split(/\s+/);
- for (const part of versionParts) {
- const version = part.replace(RANGE_OPERATOR_TRIM, '');
- if (!version || hasForbiddenLeadingVPrefix(version)) {
- return false;
- }
- }
return true;
};Add unit tests covering "v1.0.0", ">=v1.0.0 <2.0.0", ">= v1.0.0 < 2.0.0", and "v1.0.0 - v2.0.0".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isValidCatalogSemverRange = (value: string): boolean => { | |
| const trimmed = value.trim(); | |
| if (!trimmed) { | |
| return false; | |
| } | |
| const normalizedRange = semver.validRange(trimmed); | |
| if (normalizedRange === null) { | |
| return false; | |
| } | |
| const versionParts = normalizedRange.split(/\s+/); | |
| for (const part of versionParts) { | |
| const version = part.replace(RANGE_OPERATOR_TRIM, ''); | |
| if (!version || hasForbiddenLeadingVPrefix(version)) { | |
| return false; | |
| } | |
| return semver.valid(value) !== null; | |
| }); | |
| } | |
| return true; | |
| }; | |
| const isValidCatalogSemverRange = (value: string): boolean => { | |
| const trimmed = value.trim(); | |
| if (!trimmed) { | |
| return false; | |
| } | |
| // Check for leading "v" in the original input *before* semver strips it away. | |
| for (const part of trimmed.split(/\s+/)) { | |
| const operand = part.replace(RANGE_OPERATOR_TRIM, ''); | |
| if (hasForbiddenLeadingVPrefix(operand)) { | |
| return false; | |
| } | |
| } | |
| const normalizedRange = semver.validRange(trimmed); | |
| if (normalizedRange === null) { | |
| return false; | |
| } | |
| return true; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/ui-components/src/components/Catalog/AddCatalogItemWizard/utils.ts`
around lines 358 - 375, The validator is checking for a leading "v" after
calling semver.validRange(), so inputs like "v1.0.0" slip through; update
isValidCatalogSemverRange to first inspect the original trimmed input for
forbidden leading "v" on each token (use RANGE_OPERATOR_TRIM to strip
operators/spaces and hasForbiddenLeadingVPrefix to detect the prefix) and return
false if any token has a leading "v" before calling semver.validRange(), then
proceed with normalization and the existing checks; also add unit tests for
"v1.0.0", ">=v1.0.0 <2.0.0", ">= v1.0.0 < 2.0.0", and "v1.0.0 - v2.0.0" to cover
these cases.
2 participants
The "version" field in the UI accepted a "v" prefix to the semver version, which is invalid.
The helper text even suggested v2.1.0-rc1 as a valid version, which it is not.
Includes adaptations to API changes done in flightctl/flightctl#2687 which is now already merged.
Summary by CodeRabbit
Bug Fixes
Documentation