Fix: Updates for VNC ports #2505
Conversation
WalkthroughThe changes update VM manager port configuration and display formatting. The VMRC connection string now includes the port in its output, while GPU WebSocket port defaults shift from 5901 to 5700 with corresponding validation range updates. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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 |
🔧 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: 0
🧹 Nitpick comments (1)
emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php (1)
2131-2142: Validation logic is correct; consider improving error message clarity.The JavaScript validation function correctly enforces the port range constraints and prevents port conflicts. The logic at line 2134 properly validates:
- VNC/SPICE port: 5900–65535
- WS port: 5700–5899
- Ports must not be equal
However, the combined error message at line 2137 addresses multiple failure conditions at once. Consider providing more specific feedback based on which validation rule failed (e.g., separate messages for range violations vs. duplicate ports).
🔎 Optional refactor for clearer error messages
function checkVNCPorts() { const port = $("#port").val(); const wsport = $("#wsport").val(); - if (port < 5900 || port > 65535 || wsport < 5700 || wsport > 5899 || port == wsport) { + let errorMsg = ""; + if (port < 5900 || port > 65535) { + errorMsg = "_(VNC/SPICE port must be between 5900 and 65535)_"; + } else if (wsport < 5700 || wsport > 5899) { + errorMsg = "_(WS port must be between 5700 and 5899)_"; + } else if (port == wsport) { + errorMsg = "_(VNC/SPICE port and WS port cannot be equal)_"; + } + + if (errorMsg) { swal({ title: "_(Invalid Port)_", - text: "_(VNC/SPICE ports must be between 5900 and 65535, and cannot be equal to each other. WS port should be between 5700 and 5899)_", + text: errorMsg, type: "error", confirmButtonText: "_(Ok)_" }); } }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/dynamix.vm.manager/include/VMMachines.phpemhttp/plugins/dynamix.vm.manager/templates/Custom.form.php
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Squidly271
Repo: unraid/webgui PR: 2333
File: emhttp/plugins/dynamix/include/publish.php:28-36
Timestamp: 2025-08-13T03:13:53.283Z
Learning: In the unraid/webgui codebase, PR #2333 by Squidly271 supersedes earlier work that already addressed all legacy publish() calls using the old overloaded syntax. There are no remaining legacy calls that need backward compatibility handling in the publish() function.
Learnt from: ljm42
Repo: unraid/webgui PR: 2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
📚 Learning: 2025-12-28T15:54:58.673Z
Learnt from: mgutt
Repo: unraid/webgui PR: 2496
File: emhttp/plugins/dynamix/Browse.page:901-906
Timestamp: 2025-12-28T15:54:58.673Z
Learning: In the unraid/webgui codebase, CSRF validation is centralized in the global auto_prepend_file (local_prepend.php) which runs before every PHP request. Do not add per-script CSRF checks in individual files like Browse.page or Control.php. If a script relies on global validation, ensure it does not duplicate CSRF logic; otherwise extend the central preface to cover the needed checks.
Applied to files:
emhttp/plugins/dynamix.vm.manager/templates/Custom.form.phpemhttp/plugins/dynamix.vm.manager/include/VMMachines.php
🔇 Additional comments (3)
emhttp/plugins/dynamix.vm.manager/include/VMMachines.php (1)
98-98: LGTM! Port number now visible in display string.The addition of the port number to the graphics display string improves visibility for users. The format change from
PROTOCOLDRIVERtoPROTOCOL:PORTDRIVERis clear and consistent with standard port notation.emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php (2)
108-108: Verify the rationale for the default WS port change.The default
wsporthas been changed from 5901 to 5700. This represents a significant shift in the default port allocation strategy. Please confirm:
- Is this change intentional and documented?
- What is the rationale for choosing 5700 as the new default (previously it was 5901, just above the VNC port range)?
- Are there any compatibility considerations for existing deployments?
1334-1334: Clarify the distinction between port and wsport—these are separate port types with different purposes.The port (VNC/SPICE) and wsport (WebSocket) are two independent port ranges: port ranges from 5900–65535 for VNC/SPICE protocol connections, while wsport is limited to 5700–5899 for WebSocket proxy connections. The 5700–5899 range for wsport is the original design (default wsport=5700), not a recent restriction. No breaking change has occurred—these are separate port allocations by design.
Likely an incorrect or invalid review comment.
3 participants
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.