VM template fixes#2020
Conversation
- fix show/hide elements - fix translations
|
Warning Rate limit exceeded@bergware has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes update the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php (3)
1285-1287: Improved port input field handlingThe port and WebSocket port input fields now use semantic class names for visibility control. Consider adding input validation to ensure valid port numbers are entered.
-<input id="port" type="number" size="5" maxlength="5" class="trim second <?=$hiddenport?>" name="gpu[<?=$i?>][port]" value="<?=$arrGPU['port']?>"> +<input id="port" type="number" size="5" maxlength="5" min="1" max="65535" class="trim second <?=$hiddenport?>" name="gpu[<?=$i?>][port]" value="<?=$arrGPU['port']?>"> -<input id="wsport" type="number" size="5" maxlength="5" class="trim second <?=$hiddenwsport?>" name="gpu[<?=$i?>][wsport]" value="<?=$arrGPU['wsport']?>"> +<input id="wsport" type="number" size="5" maxlength="5" min="1" max="65535" class="trim second <?=$hiddenwsport?>" name="gpu[<?=$i?>][wsport]" value="<?=$arrGPU['wsport']?>">
2146-2163: Enhanced port configuration visibilityThe
AutoportChangefunction has been improved to handle port visibility more consistently. However, consider adding error handling for invalid port configurations.Add validation to ensure port numbers don't conflict when manually configured:
function AutoportChange(autoport) { $("#port").removeClass('hidden'); $("#Porttext").removeClass('hidden'); $("#wsport").removeClass('hidden'); $("#WSPorttext").removeClass('hidden'); if (autoport.value == "yes") { $("#port").addClass('hidden'); $("#Porttext").addClass('hidden'); $("#wsport").addClass('hidden'); $("#WSPorttext").addClass('hidden'); } else { var protocol = document.getElementById("protocol").value; + // Validate port numbers when manually configured + $("#port").on('change', function() { + var port = parseInt($(this).val()); + var wsport = parseInt($("#wsport").val()); + if (port === wsport) { + alert("Port and WebSocket port must be different"); + $(this).val(''); + } + }); if (protocol != "vnc") { $("#wsport").addClass('hidden'); $("#WSPorttext").addClass('hidden'); } } }
2694-2696: Improved template creation dialogThe template creation dialog text has been updated to be more descriptive. Consider adding validation for the template name to prevent invalid characters.
swal({ title: "_(Template Name)_", text: "_(Enter name)_:\n_(If name already exists it will be replaced)_.", type: "input", showCancelButton: true, closeOnConfirm: false, + inputValidator: (value) => { + if (!/^[a-zA-Z0-9-_]+$/.test(value)) { + return 'Template name can only contain letters, numbers, hyphens and underscores'; + } + }, inputPlaceholder: "_(Leaving blank will use OS name)_." },Also applies to: 2700-2700
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php(18 hunks)
🔇 Additional comments (5)
emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php (5)
350-354: Improved element identification with semantic IDThe addition of
id="zfs-name"to the<tr>element improves element identification and accessibility. The warning message has also been updated to be more descriptive.
2036-2047: Improved SSD flag handlingThe
BusChangeandupdateSSDCheckfunctions have been updated to handle SSD flags more consistently. The code now properly toggles visibility of SSD-related elements based on the bus type.
2049-2056: Improved BIOS change handlingThe
BIOSChangefunction now properly handles USB boot options visibility based on the BIOS type. The code uses consistent class manipulation for showing/hiding elements.
2165-2178: Improved video driver configurationThe
VMConsoleDriverChangefunction now handles rendering options visibility more consistently. The code uses proper class manipulation for showing/hiding elements based on the selected driver.
916-917:Details
✅ Verification successful
Verify disk bus change handling
The
BusChangefunction now accepts an index parameter to handle multiple disks correctly. However, ensure that all existing calls to this function have been updated to pass the required index parameter.Also applies to: 927-927
🏁 Script executed:
#!/bin/bash # Search for any calls to BusChange that don't pass an index parameter rg -A 2 "BusChange\([^,]+"Length of output: 2152
Disk Bus Change Handling Verified
The updated file (
emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php) correctly callsBusChangewith two parameters (e.g., using<?=$i?>and{{INDEX}}), and the function itself is defined asfunction BusChange(value, index) { … }. This confirms that the changes on lines 916–917 and 927 are in line with the intention of handling multiple disks.• Verified that the select elements now invoke
BusChange(this.value,<?=$i?>)andBusChange(this.value,{{INDEX}}).
• Confirmed that the correspondingBusChangedefinition accepts bothvalueandindexparameters.
• Note: The legacy file (Custom.formold.php) still contains outdated calls (e.g.,BusChange(this)), which you may want to review if that file remains in active use.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php (6)
350-357: Improve error message visibility handling.The error message container for ZFS name validation has been updated to use class-based visibility control. While this works, consider adding ARIA attributes for better accessibility.
-<tr id="zfs-name" class="hidden"> +<tr id="zfs-name" class="hidden" role="alert" aria-live="polite">
2036-2043: Improve class manipulation for disk rotation UI.The code uses jQuery's addClass/removeClass for toggling visibility. Consider using classList for better performance.
-$('input[id="disk['+index+'][rotation]"]').removeClass('hidden'); -$('span[id="disk['+index+'][rotatetext]"]').removeClass('hidden'); +document.querySelector('input[id="disk['+index+'][rotation]"]').classList.remove('hidden'); +document.querySelector('span[id="disk['+index+'][rotatetext]"]').classList.remove('hidden');
2046-2047: Simplify SSD checkbox value update.The current implementation uses jQuery to get the checked state. This can be simplified.
-ssd.value = $(ssd).prop('checked') ? "1" : "0"; +ssd.value = ssd.checked ? "1" : "0";
2627-2634: Improve translation string handling.Translation strings are properly marked with
_()function. However, consider using template literals for better readability.-title: "_(Template Name)_", -text: "_(Enter name)_:\n_(If name already exists it will be replaced)_.", -inputPlaceholder: "_(Leaving blank will use OS name)_." +title: _("Template Name"), +text: `${_("Enter name")}:\n${_("If name already exists it will be replaced")}.`, +inputPlaceholder: _("Leaving blank will use OS name")
1471-1474: Improve conditional rendering logic.The code uses class manipulation for visibility. Consider using data attributes for better maintainability.
-if (mylabel == "_(None)_") $("#gpubootvga"+myindex).removeClass().addClass('hidden'); +if (mylabel == "_(None)_") { + const element = document.querySelector("#gpubootvga"+myindex); + element.dataset.visible = 'false'; + element.classList.add('hidden'); +}
926-927: Improve checkbox initialization.The code mixes HTML attributes and JavaScript for checkbox state. Consider using a single source of truth.
-<input type="checkbox" id="disk[<?=$i?>][rotation]" class="rotation <?=$ssddisabled?>" onchange="updateSSDCheck(this)" name="disk[<?=$i?>][rotation]" <?=$arrDisk['rotation'] ? "checked ":"";?> value="<?=$arrDisk['rotation']?>"> +<input type="checkbox" id="disk[<?=$i?>][rotation]" class="rotation <?=$ssddisabled?>" onchange="updateSSDCheck(this)" name="disk[<?=$i?>][rotation]" data-initial-state="<?=$arrDisk['rotation']?>" value="<?=$arrDisk['rotation']?>">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php(18 hunks)
🔇 Additional comments (3)
emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php (3)
1036-1042: LGTM! Improved dropdown options handling.The code properly handles manual option selection in disk configuration.
2049-2056: LGTM! Clean implementation of BIOS change handler.The function properly handles USB boot options visibility based on BIOS selection.
749-751: LGTM! Clear documentation for USB Controller options.The code includes well-written documentation explaining USB controller options and OS compatibility.
| <input id="port" type="number" size="5" maxlength="5" class="trim second <?=$hiddenport?>" name="gpu[<?=$i?>][port]" value="<?=$arrGPU['port']?>"> | ||
| <span id="WSPorttext" class="label <?=$hiddenwsport?>">_(VM Console WS Port)_:</span> | ||
| <input type="number" size="5" maxlength="5" id="wsport" class="trim second <?=$hiddenwsport?>" name="gpu[<?=$i?>][wsport]" value="<?=$arrGPU['wsport']?>"> | ||
| <input id="wsport" type="number" size="5" maxlength="5" class="trim second <?=$hiddenwsport?>" name="gpu[<?=$i?>][wsport]" value="<?=$arrGPU['wsport']?>"> | ||
| </td> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify port number validation.
The code accepts port numbers without validation. Consider adding range checks for valid port numbers.
🏁 Script executed:
#!/bin/bash
# Check for any port number validation in the codebase
rg -A 2 'port.*=.*value' | grep -E 'if.*port.*[<>]'Length of output: 102483
Action Required: Missing Range Validation for Port Numbers
The input fields for the GPU port numbers in this file currently lack any range validation checks. Unlike similar port inputs elsewhere (e.g., in WGX.page and WG0.page where explicit min/max attributes are used), these fields do not enforce valid port values through additional attributes or conditional checks. Please consider adding proper validation—either via HTML attributes (such as setting min="1" and max="65535") or through server‐side/client‐side logic—to ensure only valid port numbers are accepted.
- File: emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php (Lines 1285–1288)
- Suggestion: Add range checks by:
- Including HTML attributes (
minandmax) in the<input>elements. - Implementing appropriate validation logic in any supporting JavaScript or PHP that processes these values.
- Including HTML attributes (
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php (1)
1286-1289:⚠️ Potential issueAdd port number validation.
The code accepts port numbers without validation, which could lead to invalid configurations.
Add proper validation for port numbers to ensure they are within valid ranges (1-65535).
🧹 Nitpick comments (6)
emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php (6)
350-358: Improve warning message visibility handling.The code uses direct class manipulation for showing/hiding warning messages. Consider using a dedicated function to handle visibility state changes consistently.
- <tr id="zfs-name" class="hidden"> + <tr id="zfs-name" class="warning-message hidden">And add a utility function:
function toggleWarningVisibility(elementId, show) { const element = document.getElementById(elementId); if (element) { element.classList.toggle('hidden', !show); } }
1038-1045: Improve disk bus change handler.The function uses direct DOM manipulation for showing/hiding SSD-related elements. Consider using a more maintainable approach with data attributes.
-function BusChange(value, index) { - $('input[id="disk['+index+'][rotation]"]').removeClass('hidden'); - $('span[id="disk['+index+'][rotatetext]"]').removeClass('hidden'); - if (value == "virtio" || value == "usb" ) { - $('input[id="disk['+index+'][rotation]"]').addClass('hidden'); - $('span[id="disk['+index+'][rotatetext]"]').addClass('hidden'); - } -} +function BusChange(value, index) { + const hideSSD = value === "virtio" || value === "usb"; + const elements = document.querySelectorAll(`[data-disk-index="${index}"][data-ssd-element]`); + elements.forEach(el => el.classList.toggle('hidden', hideSSD)); +}
2047-2049: Simplify SSD check update handler.The function can be simplified to a one-liner using the checked property directly.
-function updateSSDCheck(ssd) { - ssd.value = $(ssd).prop('checked') ? "1" : "0"; -} +const updateSSDCheck = ssd => ssd.value = ssd.checked ? "1" : "0";
2051-2058: Improve BIOS change handler.The function uses direct DOM manipulation for showing/hiding USB boot elements. Consider using a more maintainable approach.
-function BIOSChange(value) { - $("#USBBoottext").removeClass('hidden'); - $("#domain_usbboot").removeClass('hidden'); - if (value == "0") { - $("#USBBoottext").addClass('hidden'); - $("#domain_usbboot").addClass('hidden'); - } -} +function BIOSChange(value) { + const showUSBBoot = value !== "0"; + ["USBBoottext", "domain_usbboot"].forEach(id => { + document.getElementById(id)?.classList.toggle('hidden', !showUSBBoot); + }); +}
2148-2165: Improve autoport change handler.The function has redundant class manipulations and can be simplified.
-function AutoportChange(autoport) { - $("#port").removeClass('hidden'); - $("#Porttext").removeClass('hidden'); - $("#wsport").removeClass('hidden'); - $("#WSPorttext").removeClass('hidden'); - if (autoport.value == "yes") { - $("#port").addClass('hidden'); - $("#Porttext").addClass('hidden'); - $("#wsport").addClass('hidden'); - $("#WSPorttext").addClass('hidden'); - } else { - var protocol = document.getElementById("protocol").value; - if (protocol != "vnc") { - $("#wsport").addClass('hidden'); - $("#WSPorttext").addClass('hidden'); - } - } -} +function AutoportChange(autoport) { + const elements = ["port", "Porttext", "wsport", "WSPorttext"]; + const hideAll = autoport.value === "yes"; + const protocol = document.getElementById("protocol").value; + + elements.forEach(id => { + const el = document.getElementById(id); + if (el) { + const isWSElement = id.toLowerCase().includes('ws'); + el.classList.toggle('hidden', hideAll || (protocol !== "vnc" && isWSElement)); + } + }); +}
2167-2180: Improve VM console driver change handler.The function has redundant class manipulations and can be simplified.
-function VMConsoleDriverChange(driver) { - $("#vncrender").removeClass('hidden'); - $("#vncrendertext").removeClass('hidden'); - if (driver.value != "virtio3d") { - $("#vncrender").addClass('hidden'); - $("#vncrendertext").addClass('hidden'); - } - $("#vncdspopt").removeClass('hidden'); - $("#vncdspopttext").removeClass('hidden'); - if (driver.value != "qxl") { - $("#vncdspopt").addClass('hidden'); - $("#vncdspopttext").addClass('hidden'); - } -} +function VMConsoleDriverChange(driver) { + const elements = { + render: ["vncrender", "vncrendertext"], + display: ["vncdspopt", "vncdspopttext"] + }; + + elements.render.forEach(id => { + document.getElementById(id)?.classList.toggle('hidden', driver.value !== "virtio3d"); + }); + + elements.display.forEach(id => { + document.getElementById(id)?.classList.toggle('hidden', driver.value !== "qxl"); + }); +}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/include/Wireless.php (1)
77-95: Consider simplifying the array initialization logic.While the logic is correct, it could be more concise by using early returns.
Consider this alternative:
- if ($load && count(array_keys($wifi)) > 1) { - foreach ($wifi as $network => $block) { - if ($network == $port) continue; - $wlan[$index]['bss'] = $block['ATTR1']; - $wlan[$index]['signal'] = $block['ATTR2']; - $wlan[$index]['security'] = $block['ATTR3'] ?? $block['SECURITY']; - $wlan[$index]['ssid'] = $network; - $index++; - } - $index = 0; - } else { - $wlan = scanWifi($port); - } + if (!$load || count(array_keys($wifi)) <= 1) { + return scanWifi($port); + } + + $wlan = []; + foreach ($wifi as $network => $block) { + if ($network == $port) continue; + $wlan[] = [ + 'bss' => $block['ATTR1'], + 'signal' => $block['ATTR2'], + 'security' => $block['ATTR3'] ?? $block['SECURITY'], + 'ssid' => $network + ]; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/dynamix/Wireless.page(2 hunks)emhttp/plugins/dynamix/include/Wireless.php(2 hunks)
🔇 Additional comments (4)
emhttp/plugins/dynamix/Wireless.page (2)
67-77: LGTM! Performance and UX improvements.The changes improve the function by:
- Adding load parameter for optimized scanning
- Reducing update interval for better responsiveness
- Adding conditional rendering to prevent empty sections
149-149: LGTM! Proper initialization.The change correctly passes the load parameter for initial WiFi scanning.
emhttp/plugins/dynamix/include/Wireless.php (2)
97-97: LGTM! Improved error handling.The change from
file_existstois_readableimproves security by ensuring both file existence and proper permissions.
122-122: LGTM! Better empty state handling.The change improves UX by preventing the "None" message from flickering during initial load.
- make separate warnings for snapshots and user input - widen vncrender and vncdspopt selection
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/scripts/wireless (1)
34-34: Consider improving readability of the complex condition.While the null coalescing operator improves robustness, the condition combines multiple checks that could be more readable if split:
- if (($block['GROUP']??'') == $state && (!$arg || $arg == $network)) { + $groupState = $block['GROUP'] ?? ''; + $isTargetNetwork = !$arg || $arg == $network; + if ($groupState == $state && $isTargetNetwork) {This refactoring:
- Makes the logic more self-documenting
- Maintains the same robust behavior
- Separates concerns for easier maintenance
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/dynamix/scripts/wireless(1 hunks)etc/rc.d/rc.docker(1 hunks)
🔇 Additional comments (2)
etc/rc.d/rc.docker (1)
471-477: LGTM! Improved network interface cleanup.The changes correctly move the
VHOSTassignment into theelseblock and add proper cleanup of IP addresses for existing vhost interfaces when host access is disabled. This ensures that network interfaces are properly managed and prevents potential IP address conflicts.emhttp/plugins/dynamix/scripts/wireless (1)
31-31: LGTM! Improved control flow with defensive programming.The addition of the null coalescing operator and explicit wireless check enhances robustness by ensuring the configuration block only executes when wireless is actually enabled.
3 participants
Summary by CodeRabbit
Style
.hiddento control element visibility more effectively.Refactor
Bug Fixes
New Features