Copilot commented on code in PR #14413:
URL: https://github.com/apache/cloudstack/pull/14413#discussion_r4518104904
##########
ui/src/views/compute/wizard/ComputeSelection.vue:
##########
@@ -57,23 +57,42 @@
</a-col>
<a-col :md="colContraned" :lg="colContraned" v-if="isCustomized">
<a-form-item
- :label="$t('label.memory.mb')"
+ :label="$t('label.memory')"
:validate-status="errors.memory.status"
:help="errors.memory.message">
<a-row :gutter="12">
- <a-col :md="10" :lg="10" v-show="isConstrained && maxMemory &&
!isNaN(maxMemory)">
+ <a-col :md="14" :lg="14" v-show="isConstrained && maxMemory &&
!isNaN(maxMemory) && maxMemory < 1000000000">
Review Comment:
The same long predicate is duplicated in multiple bindings, and the
`1000000000` threshold is a magic number. Consider extracting a computed
boolean (e.g., `showMemorySlider`) and a named constant (or computed) for the
threshold (with a short comment describing why the cutoff exists), then reuse
it in both places to reduce repetition and make future changes safer.
##########
ui/src/views/compute/wizard/ComputeSelection.vue:
##########
@@ -57,23 +57,42 @@
</a-col>
<a-col :md="colContraned" :lg="colContraned" v-if="isCustomized">
<a-form-item
- :label="$t('label.memory.mb')"
+ :label="$t('label.memory')"
:validate-status="errors.memory.status"
:help="errors.memory.message">
<a-row :gutter="12">
- <a-col :md="10" :lg="10" v-show="isConstrained && maxMemory &&
!isNaN(maxMemory)">
+ <a-col :md="14" :lg="14" v-show="isConstrained && maxMemory &&
!isNaN(maxMemory) && maxMemory < 1000000000">
<a-slider
- :min="minMemory"
- :max="maxMemory"
- v-model:value="memoryInputValue"
+ :min="minMemoryInUnit"
+ :max="maxMemoryInUnit"
+ :step="memoryStep"
+ v-model:value="memoryDisplayValue"
@change="($event) => updateComputeMemory($event)"
/>
</a-col>
- <a-col :md="4" :lg="4">
- <a-input-number
- v-model:value="memoryInputValue"
- @change="($event) => updateComputeMemory($event)"
- />
+ <a-col :md="isConstrained && maxMemory && !isNaN(maxMemory) &&
maxMemory < 1000000000 ? 10 : 24" :lg="isConstrained && maxMemory &&
!isNaN(maxMemory) && maxMemory < 1000000000 ? 10 : 24">
Review Comment:
The same long predicate is duplicated in multiple bindings, and the
`1000000000` threshold is a magic number. Consider extracting a computed
boolean (e.g., `showMemorySlider`) and a named constant (or computed) for the
threshold (with a short comment describing why the cutoff exists), then reuse
it in both places to reduce repetition and make future changes safer.
##########
ui/src/views/compute/wizard/ComputeSelection.vue:
##########
@@ -192,6 +215,47 @@ export default {
}
return 8
+ },
+ isGiBAllowed () {
+ if (this.preFillContent && this.preFillContent.memory &&
(this.preFillContent.memory % 256 !== 0)) {
+ return false
+ }
+ if (this.initialMemoryValue > 0 && (this.initialMemoryValue % 256 !==
0)) {
+ return false
+ }
+ if (!this.isConstrained) {
+ return true
+ }
+ const isMinValid = !this.minMemory || (this.minMemory >= 256 &&
this.minMemory % 256 === 0)
+ const isMaxValid = !this.maxMemory || this.maxMemory >=
Number.MAX_SAFE_INTEGER || (this.maxMemory >= 256 && this.maxMemory % 256 === 0)
+ return Boolean(isMinValid && isMaxValid)
+ },
+ minMemoryInUnit () {
+ if (!this.minMemory) return 0
+ if (this.memoryUnit === 'GiB') {
+ return this.minMemory / 1024
+ }
+ return this.minMemory
+ },
+ maxMemoryInUnit () {
+ if (!this.maxMemory || this.maxMemory >= Number.MAX_SAFE_INTEGER) return 0
+ if (this.memoryUnit === 'GiB') {
+ return this.maxMemory / 1024
+ }
+ return this.maxMemory
+ },
+ memoryStep () {
+ if (this.memoryUnit === 'GiB') {
+ return 0.25
+ }
+ return 1
+ },
Review Comment:
The implementation hard-codes GiB stepping to 0.25 (and validates multiples
of 0.25), but the PR description says GiB mode uses a 0.01 step (or smaller for
sub‑GiB) and rounds display to 2 decimals. Either update the PR description to
match the enforced 0.25 GiB increments, or update the code to implement the
described 0.01/dynamic step behavior (and align validation accordingly).
##########
ui/src/views/compute/wizard/ComputeSelection.vue:
##########
@@ -57,23 +57,42 @@
</a-col>
<a-col :md="colContraned" :lg="colContraned" v-if="isCustomized">
<a-form-item
- :label="$t('label.memory.mb')"
+ :label="$t('label.memory')"
:validate-status="errors.memory.status"
:help="errors.memory.message">
<a-row :gutter="12">
- <a-col :md="10" :lg="10" v-show="isConstrained && maxMemory &&
!isNaN(maxMemory)">
+ <a-col :md="14" :lg="14" v-show="isConstrained && maxMemory &&
!isNaN(maxMemory) && maxMemory < 1000000000">
<a-slider
- :min="minMemory"
- :max="maxMemory"
- v-model:value="memoryInputValue"
+ :min="minMemoryInUnit"
+ :max="maxMemoryInUnit"
+ :step="memoryStep"
+ v-model:value="memoryDisplayValue"
@change="($event) => updateComputeMemory($event)"
/>
</a-col>
- <a-col :md="4" :lg="4">
- <a-input-number
- v-model:value="memoryInputValue"
- @change="($event) => updateComputeMemory($event)"
- />
+ <a-col :md="isConstrained && maxMemory && !isNaN(maxMemory) &&
maxMemory < 1000000000 ? 10 : 24" :lg="isConstrained && maxMemory &&
!isNaN(maxMemory) && maxMemory < 1000000000 ? 10 : 24">
+ <a-input-group compact style="display: flex">
+ <a-tooltip :title="memoryTooltipText" placement="topLeft"
style="flex: 1; display: flex">
+ <a-input-number
+ style="width: 100%; min-width: 60px"
+ :min="isConstrained ? minMemoryInUnit : 0"
+ :max="isConstrained && maxMemoryInUnit > 0 ?
maxMemoryInUnit : undefined"
+ :step="memoryStep"
+ :precision="memoryUnit === 'GiB' ? 2 : 0"
+ v-model:value="memoryDisplayValue"
+ @change="($event) => updateComputeMemory($event)"
+ />
+ </a-tooltip>
+ <a-select
+ v-model:value="memoryUnit"
+ style="width: 75px"
+ @change="handleUnitChange">
+ <a-select-option value="GiB" :title="!isGiBAllowed ?
$t('message.memory.gib.not.supported') : undefined">
Review Comment:
When GiB isn’t allowed, the UI still allows selecting “GiB” and then reverts
with a warning in `handleUnitChange`. Consider disabling the GiB option (e.g.,
via a `:disabled="!isGiBAllowed"` binding) so users can’t select an invalid
unit in the first place; this avoids state churn, repeated `$nextTick` flips,
and warning noise.
##########
ui/src/views/compute/wizard/ComputeSelection.vue:
##########
@@ -224,40 +303,95 @@ export default {
if (this.preFillContent.cpuspeed) {
this.cpuSpeedInputValue = this.preFillContent.cpuspeed
}
- if (this.preFillContent.memory) {
- this.memoryInputValue = this.preFillContent.memory
- }
+
this.updateComputeCpuNumber(this.preFillContent.cpunumber ||
this.cpuNumberInputValue)
this.updateComputeCpuSpeed(this.preFillContent.cpuspeed ||
this.cpuSpeedInputValue)
- this.updateComputeMemory(this.preFillContent.memory ||
this.memoryInputValue)
+ // Emit exact prefilled MiB without any conversion on load
+ this.$emit('update-compute-memory', this.memoryInputDecorator,
this.memoryInputValue)
+ },
+ handleUnitChange (newUnit) {
+ if (newUnit === 'GiB') {
+ const currentMiB = this.memoryUnit === 'MiB' ? this.memoryDisplayValue
: this.memoryInputValue
+ if (!this.isGiBAllowed || (currentMiB > 0 && currentMiB % 256 !== 0)) {
+ this.memoryUnit = 'MiB'
+ this.$nextTick(() => {
+ this.memoryUnit = 'MiB'
+ })
+ message.warning(
+ this.$t('message.memory.gib.not.supported') ||
+ 'GiB unit is not available because memory limits or values must be
multiples of 256 MiB (0.25 GiB). Please use MiB.'
+ )
+ return
+ }
+ }
+
+ if (newUnit === 'MiB') {
+ this.memoryDisplayValue = (this.memoryInputValue !== undefined &&
this.memoryInputValue !== null)
+ ? this.memoryInputValue
+ : (this.memoryDisplayValue ? Math.round(this.memoryDisplayValue *
1024) : undefined)
+ } else {
+ this.memoryDisplayValue = (this.memoryInputValue !== undefined &&
this.memoryInputValue !== null)
+ ? this.memoryInputValue / 1024
+ : (this.memoryDisplayValue ? this.memoryDisplayValue : undefined)
+ }
+ if (this.isConstrained) {
+ if (this.memoryDisplayValue < this.minMemoryInUnit) {
+ this.memoryDisplayValue = this.minMemoryInUnit
+ } else if (this.maxMemoryInUnit > 0 && this.memoryDisplayValue >
this.maxMemoryInUnit) {
+ this.memoryDisplayValue = this.maxMemoryInUnit
+ }
+ }
+ this.updateComputeMemory(this.memoryDisplayValue)
},
updateComputeCpuNumber (value) {
- if (!value) this.cpuNumberInputValue = 0
+ this.cpuNumberInputValue = value
if (!this.validateInput('cpu', value)) {
+ this.$emit('update-compute-cpunumber', this.cpuNumberInputDecorator,
undefined)
return
}
this.$emit('update-compute-cpunumber', this.cpuNumberInputDecorator,
value)
},
updateComputeCpuSpeed (value) {
this.$emit('update-compute-cpuspeed', this.cpuSpeedInputDecorator, value)
},
- updateComputeMemory (value) {
- if (!value) this.memoryInputValue = 0
- if (!this.validateInput('memory', value)) {
+ updateComputeMemory (displayValue) {
+ this.memoryDisplayValue = displayValue
+ if (!this.validateInput('memory', displayValue)) {
+ this.memoryInputValue = undefined
+ this.$emit('update-compute-memory', this.memoryInputDecorator,
undefined)
return
}
- this.$emit('update-compute-memory', this.memoryInputDecorator, value)
+ let memoryInMiB
+ if (this.memoryUnit === 'GiB') {
+ memoryInMiB = Math.round(displayValue * 1024)
+ } else {
+ memoryInMiB = Math.round(displayValue)
+ }
+
Review Comment:
Using `Math.round(displayValue * 1024)` in GiB mode can yield a MiB value
that is slightly *less* than the user-entered GiB amount (e.g., 1.01 GiB →
1034.24 MiB → 1034 MiB). If the intent is “at least the requested GiB”, prefer
ceiling in GiB mode (while still clamping to max constraints). Also, the PR
description mentions “Exact Boundary Protection” for min/max constraints, but
this method doesn’t currently map values at the displayed min/max back to the
exact `minMemory`/`maxMemory` MiB values; consider explicitly snapping when
`displayValue` is within an epsilon of `minMemoryInUnit`/`maxMemoryInUnit`.
##########
ui/src/views/compute/wizard/ComputeSelection.vue:
##########
@@ -224,40 +303,95 @@ export default {
if (this.preFillContent.cpuspeed) {
this.cpuSpeedInputValue = this.preFillContent.cpuspeed
}
- if (this.preFillContent.memory) {
- this.memoryInputValue = this.preFillContent.memory
- }
+
this.updateComputeCpuNumber(this.preFillContent.cpunumber ||
this.cpuNumberInputValue)
this.updateComputeCpuSpeed(this.preFillContent.cpuspeed ||
this.cpuSpeedInputValue)
- this.updateComputeMemory(this.preFillContent.memory ||
this.memoryInputValue)
+ // Emit exact prefilled MiB without any conversion on load
+ this.$emit('update-compute-memory', this.memoryInputDecorator,
this.memoryInputValue)
+ },
+ handleUnitChange (newUnit) {
+ if (newUnit === 'GiB') {
+ const currentMiB = this.memoryUnit === 'MiB' ? this.memoryDisplayValue
: this.memoryInputValue
+ if (!this.isGiBAllowed || (currentMiB > 0 && currentMiB % 256 !== 0)) {
+ this.memoryUnit = 'MiB'
+ this.$nextTick(() => {
+ this.memoryUnit = 'MiB'
+ })
+ message.warning(
+ this.$t('message.memory.gib.not.supported') ||
+ 'GiB unit is not available because memory limits or values must be
multiples of 256 MiB (0.25 GiB). Please use MiB.'
+ )
+ return
+ }
+ }
+
+ if (newUnit === 'MiB') {
+ this.memoryDisplayValue = (this.memoryInputValue !== undefined &&
this.memoryInputValue !== null)
+ ? this.memoryInputValue
+ : (this.memoryDisplayValue ? Math.round(this.memoryDisplayValue *
1024) : undefined)
+ } else {
+ this.memoryDisplayValue = (this.memoryInputValue !== undefined &&
this.memoryInputValue !== null)
+ ? this.memoryInputValue / 1024
+ : (this.memoryDisplayValue ? this.memoryDisplayValue : undefined)
+ }
+ if (this.isConstrained) {
+ if (this.memoryDisplayValue < this.minMemoryInUnit) {
+ this.memoryDisplayValue = this.minMemoryInUnit
+ } else if (this.maxMemoryInUnit > 0 && this.memoryDisplayValue >
this.maxMemoryInUnit) {
+ this.memoryDisplayValue = this.maxMemoryInUnit
+ }
+ }
+ this.updateComputeMemory(this.memoryDisplayValue)
},
updateComputeCpuNumber (value) {
- if (!value) this.cpuNumberInputValue = 0
+ this.cpuNumberInputValue = value
if (!this.validateInput('cpu', value)) {
+ this.$emit('update-compute-cpunumber', this.cpuNumberInputDecorator,
undefined)
return
}
this.$emit('update-compute-cpunumber', this.cpuNumberInputDecorator,
value)
},
updateComputeCpuSpeed (value) {
this.$emit('update-compute-cpuspeed', this.cpuSpeedInputDecorator, value)
},
- updateComputeMemory (value) {
- if (!value) this.memoryInputValue = 0
- if (!this.validateInput('memory', value)) {
+ updateComputeMemory (displayValue) {
+ this.memoryDisplayValue = displayValue
+ if (!this.validateInput('memory', displayValue)) {
+ this.memoryInputValue = undefined
+ this.$emit('update-compute-memory', this.memoryInputDecorator,
undefined)
return
}
- this.$emit('update-compute-memory', this.memoryInputDecorator, value)
+ let memoryInMiB
+ if (this.memoryUnit === 'GiB') {
+ memoryInMiB = Math.round(displayValue * 1024)
+ } else {
+ memoryInMiB = Math.round(displayValue)
+ }
+
+ this.memoryInputValue = memoryInMiB
+ this.$emit('update-compute-memory', this.memoryInputDecorator,
memoryInMiB)
},
validateInput (input, value) {
this.errors[input].status = ''
this.errors[input].message = ''
- if (value === null || value === undefined || value.length === 0) {
+ if (value === null || value === undefined || value.length === 0 || value
=== '' || value === 0) {
this.errors[input].status = 'error'
this.errors[input].message = this.$t('message.error.required.input')
return false
}
+ if (input === 'memory') {
+ if (this.memoryUnit === 'GiB') {
+ const isStepMultiple = Math.abs(value * 4 - Math.round(value * 4)) <
0.001
+ if (!isStepMultiple) {
+ this.errors.memory.status = 'error'
+ this.errors.memory.message =
this.$t('message.error.memory.gib.step') || 'Please enter memory in multiples
of 0.25 GiB (256 MiB), or switch to MiB.'
+ return false
+ }
+ }
+ }
Review Comment:
The implementation hard-codes GiB stepping to 0.25 (and validates multiples
of 0.25), but the PR description says GiB mode uses a 0.01 step (or smaller for
sub‑GiB) and rounds display to 2 decimals. Either update the PR description to
match the enforced 0.25 GiB increments, or update the code to implement the
described 0.01/dynamic step behavior (and align validation accordingly).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
