Feat: Add port checking for VNC and sane defaults #2452
Conversation
WalkthroughDefault GPU port values updated from -1 to 5900 and 5901. Form inputs enhanced with validation attributes. New client-side validation function added to enforce VNC port range constraints (5900-65535) and ensure port values differ. Changes
Sequence DiagramsequenceDiagram
participant User
participant Form as Form Input
participant Validation as checkVNCPorts()
participant Alert as Notification
User->>Form: Change port/wsport value
Form->>Validation: Trigger onchange event
Validation->>Validation: Check if port in [5900, 65535]
Validation->>Validation: Check if wsport in [5900, 65535]
Validation->>Validation: Check port ≠ wsport
alt Invalid Port Range or Duplicate
Validation->>Alert: Show error message
Alert->>User: Display alert
else Valid Ports
Validation->>User: ✓ Validation passed
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
🔧 PR Test Plugin AvailableA test plugin has been generated for this PR that includes the modified files. Version: 📥 Installation Instructions:Install via Unraid Web UI:
Alternative: Direct Download
|
![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
📜 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(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Squidly271
Repo: unraid/webgui PR: 0
File: :0-0
Timestamp: 2025-05-31T05:10:13.120Z
Learning: Squidly271 prefers not to receive CodeRabbit reviews or feedback while PRs are in draft status. Only provide reviews after PRs are moved out of draft.
Learnt from: Squidly271
Repo: unraid/webgui PR: 0
File: :0-0
Timestamp: 2025-09-03T07:28:01.902Z
Learning: Squidly271's preference for not reviewing drafts is specifically because they find reviews annoying during active development phases with multiple commits, but they do want reviews once the PR is ready and more stable.
🔇 Additional comments (2)
emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php (2)
106-107: LGTM! Sensible default port values.The change from -1 to 5900/5901 provides better defaults aligned with standard VNC port conventions.
1328-1330: Good addition of HTML5 validation attributes.The
min,max, andonchangeattributes provide appropriate client-side validation constraints for port inputs.
| function checkVNCPorts() { | ||
| const port = $("#port").val(); | ||
| const wsport = $("#wsport").val(); | ||
| if (port < 5900 || port > 65535 || wsport < 5900 || wsport > 65535 || port == wsport) { | ||
| swal({ | ||
| title: "_(Invalid Port)_", | ||
| text: "_(VNC/SPICE ports must be between 5900 and 65535, and cannot be equal to each other)_", | ||
| type: "error", | ||
| confirmButtonText: "_(Ok)_" | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix type coercion bug in numeric comparisons.
The function compares port values using < and > operators on strings returned by .val(), which can produce incorrect results. Additionally, the validation doesn't prevent form submission with invalid values.
Apply this diff to fix the type coercion and add form submission prevention:
function checkVNCPorts() {
- const port = $("#port").val();
- const wsport = $("#wsport").val();
- if (port < 5900 || port > 65535 || wsport < 5900 || wsport > 65535 || port == wsport) {
+ const port = parseInt($("#port").val(), 10);
+ const wsport = parseInt($("#wsport").val(), 10);
+
+ // Only validate if fields are visible (autoport is "no")
+ if ($("#port").hasClass("hidden")) {
+ return true;
+ }
+
+ if (isNaN(port) || isNaN(wsport) || port < 5900 || port > 65535 || wsport < 5900 || wsport > 65535 || port === wsport) {
swal({
title: "_(Invalid Port)_",
text: "_(VNC/SPICE ports must be between 5900 and 65535, and cannot be equal to each other)_",
type: "error",
confirmButtonText: "_(Ok)_"
});
+ return false;
}
+ return true;
}Additionally, consider validating before form submission by adding a check in the submit handlers (around lines 2662 and 2809):
// In the form submit handlers, before posting:
if (!checkVNCPorts()) {
$panel.find('input').prop('disabled', false);
$button.val($button.attr('readyvalue'));
return;
}🤖 Prompt for AI Agents
In emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php around lines 2091
to 2102, checkVNCPorts currently compares string values from .val(), causing
type coercion bugs and it doesn't prevent submission; update the function to
parse port and wsport to integers (use parseInt with base 10), validate for NaN
and the 5900–65535 range and inequality, return false when invalid (and show the
existing swal) and true when valid; then add the suggested pre-submit guard in
the form submit handlers near lines 2662 and 2809: call checkVNCPorts(), and if
it returns false re-enable inputs, reset the button value and return early to
stop the post.
2 participants
Check for VNC/Spice ports within range and set default ports to 5900/5901 instead of invalid -1
Summary by CodeRabbit
New Features
Bug Fixes
UI/UX Improvements