Fix: Remote view and RDP if no user shares are set.#2309
Conversation
WalkthroughA new variable is introduced to dynamically determine the system path based on the VM image file location. All hardcoded references to the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 4
🧹 Nitpick comments (1)
emhttp/plugins/dynamix.vm.manager/include/VMajax.php (1)
23-25: Consider refactoring to a helper function for better maintainability.The dynamic path extraction logic is used in multiple places throughout the file. Consider extracting this into a helper function to improve code reusability and make it easier to add proper error handling.
Add this helper function near the top of the file:
function getSystemPath() { global $domain_cfg; if (empty($domain_cfg['IMAGE_FILE'])) { return false; } if (preg_match('#^(.*?/system/)#', $domain_cfg['IMAGE_FILE'], $matches)) { return $matches[1]; } return false; }Then replace the current usage with:
-if (preg_match('#^(.*?/system/)#', $domain_cfg['IMAGE_FILE'], $matches)) { - $domain_system_path = $matches[1]; -} +$domain_system_path = getSystemPath();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix.vm.manager/include/VMajax.php(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
emhttp/plugins/dynamix.vm.manager/include/VMajax.php (2)
Learnt from: zackspear
PR: #2099
File: emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php:13-19
Timestamp: 2025-03-27T22:04:34.550Z
Learning: The file emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php is synced from another repository and should not be modified directly in the webgui repository.
Learnt from: zackspear
PR: #2099
File: emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php:58-74
Timestamp: 2025-03-27T22:04:00.594Z
Learning: The file emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php is synced from a different repository, and modifications should not be suggested in this repository's context. Changes should be proposed in the source repository instead.
| if (preg_match('#^(.*?/system/)#', $domain_cfg['IMAGE_FILE'], $matches)) { | ||
| $domain_system_path = $matches[1]; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify regex pattern handles edge cases properly.
The regex pattern #^(.*?/system/)# extracts the system path from the VM image file location. However, there are potential issues:
- The variable
$domain_system_pathis not initialized if the regex doesn't match - No validation that
$domain_cfg['IMAGE_FILE']exists or is not empty
Run the following script to verify how this pattern behaves with various VM image file paths:
🏁 Script executed:
#!/bin/bash
# Description: Check for potential edge cases in VM image file paths and verify regex behavior
# Search for usage of domain_cfg IMAGE_FILE to understand expected formats
rg -A 3 -B 3 "IMAGE_FILE|domain_cfg" --type php
# Look for any initialization or default values for domain_cfg
ast-grep --pattern 'domain_cfg = $_'
# Check if there are any safeguards for missing domain_cfg
rg "isset.*domain_cfg|empty.*domain_cfg" --type phpLength of output: 25814
🏁 Script executed:
#!/bin/bash
# Show the initial context of VMajax.php to confirm where and how $domain_cfg and IMAGE_FILE are defined
sed -n '1,100p' emhttp/plugins/dynamix.vm.manager/include/VMajax.phpLength of output: 3447
Ensure IMAGE_FILE is validated and $domain_system_path is always initialized
Wrap the regex in a non-empty check and provide a sensible default for $domain_system_path so it can never remain undefined:
• File: emhttp/plugins/dynamix.vm.manager/include/VMajax.php
Lines: ~23–25
Suggested diff:
- if (preg_match('#^(.*?/system/)#', $domain_cfg['IMAGE_FILE'], $matches)) {
- $domain_system_path = $matches[1];
- }
+ // Default to the standard system share
+ $domain_system_path = '/mnt/user/system/';
+ if (!empty($domain_cfg['IMAGE_FILE'])
+ && preg_match('#^(.*?/system/)#', $domain_cfg['IMAGE_FILE'], $matches)
+ ) {
+ $domain_system_path = $matches[1];
+ }This guarantees:
$domain_system_pathis always defined (no “undefined variable” errors).- We only attempt the regex when
IMAGE_FILEis set and non-empty. - Fallback remains compatible with existing Unraid conventions.
🤖 Prompt for AI Agents
In emhttp/plugins/dynamix.vm.manager/include/VMajax.php around lines 23 to 25,
ensure $domain_system_path is always initialized by first checking that
$domain_cfg['IMAGE_FILE'] is set and non-empty before running the preg_match.
Initialize $domain_system_path with a sensible default value before the
condition to avoid undefined variable errors. This ensures safe regex execution
and maintains compatibility with existing Unraid conventions.
2 participants
If system is set with no user shares. For VMs the remote view and rdp options fail. This PR resolve the issue,
Summary by CodeRabbit