Fix #2500: File Manager UI/UX improvements #2501
Conversation
WalkthroughAdds popular-destination tracking, JSON-based job queue handling, programmatic file-tree navigation and path-aware terminal support; enhances rsync progress reporting, input-driven UI interactions, and dialog behaviors while adjusting templates and styles for improved File Manager UX. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Input as "#dfm_target (input)"
participant Browse as "Browse.page"
participant Tree as "FileTree UI"
participant Dialog as "Action Dialog"
Note over User,Input: User types or selects a destination
User->>Input: enter path / pick popular
Input->>Browse: input event -> setupTargetNavigation()
Browse->>Browse: validate & sync input, decide target
Browse->>Tree: navigateFileTree(target)
activate Tree
Tree->>Tree: openFolderRecursive(parts)
Tree-->>Browse: folder nodes expanded
deactivate Tree
Browse->>Dialog: keep dialog open (preventFileTreeClose)
Note right of Dialog: Dialog hooks ensure navigation + cleanup on close
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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: 3
🧹 Nitpick comments (2)
emhttp/plugins/dynamix/include/PopularDestinations.php (1)
43-46: Consider adding error handling and file locking.The
file_put_contentscall lacks error handling and file locking. In a multi-tab or concurrent scenario, simultaneous writes could corrupt the JSON file.🔎 Proposed improvement with locking
function savePopularDestinations($data) { $json = json_encode($data, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES); - file_put_contents(POPULAR_DESTINATIONS_FILE, $json); + file_put_contents(POPULAR_DESTINATIONS_FILE, $json, LOCK_EX); }emhttp/plugins/dynamix/include/Control.php (1)
184-199: Potential index issue when undoing multiple jobs.When undoing multiple jobs (e.g., rows 1 and 3), the
unset()operations work correctly since they operate on the original array indices before removing any elements. However, afterunset(),implode("\n", $lines)will preserve gaps. Thearray_values()reindex is missing but not strictly needed since file_put_contents will join the remaining values correctly.The
array_reverseis unnecessary here since we're using direct array indices rather than sequential removal.🔎 Simplified version
case 'undo': $jobs = '/var/tmp/file.manager.jobs'; $undo = '0'; if (file_exists($jobs)) { - $rows = array_reverse(explode(',',$_POST['row'])); + $rows = explode(',',$_POST['row']); $lines = file($jobs, FILE_IGNORE_NEW_LINES); foreach ($rows as $row) { $line_number = $row - 1; // Convert 1-based job number to 0-based array index if (isset($lines[$line_number])) { unset($lines[$line_number]); } } + $lines = array_values($lines); // Re-index to remove gaps if (count($lines) > 0) { file_put_contents($jobs, implode("\n", $lines)."\n"); $undo = '2'; } else { delete_file($jobs); $undo = '1'; } } die($undo);
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
emhttp/plugins/dynamix/Browse.pageemhttp/plugins/dynamix/include/Browse.phpemhttp/plugins/dynamix/include/Control.phpemhttp/plugins/dynamix/include/FileTree.phpemhttp/plugins/dynamix/include/OpenTerminal.phpemhttp/plugins/dynamix/include/PopularDestinations.phpemhttp/plugins/dynamix/include/Templates.phpemhttp/plugins/dynamix/include/local_prepend.phpemhttp/plugins/dynamix/nchan/file_manageremhttp/plugins/dynamix/sheets/BrowseButton.cssemhttp/plugins/dynamix/styles/default-base.cssemhttp/plugins/dynamix/styles/default-dynamix.cssetc/rc.d/rc.nginx
💤 Files with no reviewable changes (2)
- emhttp/plugins/dynamix/sheets/BrowseButton.css
- emhttp/plugins/dynamix/styles/default-dynamix.css
🧰 Additional context used
🧠 Learnings (10)
📚 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/include/local_prepend.phpemhttp/plugins/dynamix/include/PopularDestinations.phpemhttp/plugins/dynamix/include/FileTree.phpemhttp/plugins/dynamix/include/Templates.phpemhttp/plugins/dynamix/include/Control.phpemhttp/plugins/dynamix/include/OpenTerminal.phpemhttp/plugins/dynamix/include/Browse.php
📚 Learning: 2025-06-18T17:09:35.579Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2264
File: emhttp/plugins/dynamix/include/.login.php:0-0
Timestamp: 2025-06-18T17:09:35.579Z
Learning: In the Unraid webGUI login system (emhttp/plugins/dynamix/include/.login.php), the cooldown timer restarting on page reload during the cooldown period is intentional behavior, not a bug. When a form is resubmitted during cooldown, the timer should restart to provide consistent user feedback.
Applied to files:
emhttp/plugins/dynamix/include/local_prepend.php
📚 Learning: 2025-09-05T16:33:12.970Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2353
File: emhttp/plugins/dynamix/DashStats.page:2257-2257
Timestamp: 2025-09-05T16:33:12.970Z
Learning: In the Unraid webGUI Nchan system, the Nchan header declaration lists publisher scripts to start (e.g., "update_1"), while the JavaScript subscription endpoints use normalized names (e.g., "/sub/update1"). The publisher script "update_1" publishes to "/pub/update1" endpoint, creating a separation between script names and published endpoints. This is by design and not a mismatch.
Applied to files:
etc/rc.d/rc.nginx
📚 Learning: 2025-10-03T02:57:29.994Z
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.
Applied to files:
etc/rc.d/rc.nginx
📚 Learning: 2025-09-05T16:33:12.970Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2353
File: emhttp/plugins/dynamix/DashStats.page:2257-2257
Timestamp: 2025-09-05T16:33:12.970Z
Learning: In the Unraid webGUI Nchan system, publisher scripts (like "update_1") publish to normalized endpoint names (like "update1"). The Nchan header lists the script names to start, while JavaScript subscribes to the published endpoints. For example: Nchan="update_1" starts the script which calls publish_noDupe('update1', data), and JavaScript subscribes to '/sub/update1'. This is the intended design, not a mismatch.
Applied to files:
etc/rc.d/rc.nginx
📚 Learning: 2025-10-04T05:22:33.141Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2421
File: emhttp/plugins/dynamix/include/DefaultPageLayout/MainContentTabless.php:23-0
Timestamp: 2025-10-04T05:22:33.141Z
Learning: In the Unraid webgui repository, `emhttp/webGui` is a symlink that points to `plugins/dynamix`. Therefore, paths using `$docroot/webGui/...` correctly resolve to `$docroot/plugins/dynamix/...` at runtime.
Applied to files:
emhttp/plugins/dynamix/include/FileTree.phpemhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-03-27T22:04:34.550Z
Learnt from: zackspear
Repo: unraid/webgui 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.
Applied to files:
emhttp/plugins/dynamix/include/FileTree.phpemhttp/plugins/dynamix/include/Templates.phpemhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-09-05T19:26:36.587Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2354
File: emhttp/plugins/dynamix/ShareEdit.page:0-0
Timestamp: 2025-09-05T19:26:36.587Z
Learning: In emhttp/plugins/dynamix/ShareEdit.page, the clone-settings div was moved outside the form element and both are wrapped in a div.relative container to prevent event bubbling issues while maintaining proper positioning.
Applied to files:
emhttp/plugins/dynamix/include/Templates.phpemhttp/plugins/dynamix/Browse.page
📚 Learning: 2025-06-21T00:10:40.789Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2258
File: emhttp/plugins/dynamix/DashStats.page:0-0
Timestamp: 2025-06-21T00:10:40.789Z
Learning: In the Unraid webgui codebase (emhttp/plugins/dynamix), replacing `<i>` elements with `<button>` elements for accessibility would require extensive CSS refactoring due to legacy CSS having direct button styles that would conflict with icon-based toggles.
Applied to files:
emhttp/plugins/dynamix/include/Templates.php
📚 Learning: 2025-06-03T21:27:15.912Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2230
File: emhttp/plugins/dynamix/include/Templates.php:63-74
Timestamp: 2025-06-03T21:27:15.912Z
Learning: In the Unraid WebGUI codebase (emhttp/plugins/dynamix/include/Templates.php), there are known duplicate ID issues in checkbox templates across multiple template instances that the maintainers are aware of but have chosen not to address due to the effort required for legacy code improvements.
Applied to files:
emhttp/plugins/dynamix/include/Templates.php
🧬 Code graph analysis (3)
emhttp/plugins/dynamix/include/FileTree.php (1)
emhttp/plugins/dynamix/include/PopularDestinations.php (1)
getPopularDestinations(123-138)
emhttp/plugins/dynamix/include/Templates.php (1)
emhttp/plugins/dynamix/include/Helpers.php (1)
mk_option(182-184)
emhttp/plugins/dynamix/include/Control.php (1)
emhttp/plugins/dynamix/include/PopularDestinations.php (1)
updatePopularDestinations(52-116)
🔇 Additional comments (29)
etc/rc.d/rc.nginx (1)
415-415: Fix nchan subscriber endpoint regex to prevent unintended path matches.The
^anchor is functionally necessary, not optional. Nginx regex location blocks do not implicitly anchor at the start of the URI. Without^, the pattern would match any URI containing/sub/anywhere in the path (e.g.,/api/sub/fileor/dashboard/sub/item), routing those requests incorrectly to the nchan subscriber endpoint. The explicit^anchor correctly restricts matches to URIs that begin with/sub/.emhttp/plugins/dynamix/include/local_prepend.php (2)
33-38: LGTM: Improved readability.The multi-line formatting of the conditional improves readability without changing logic.
42-50: Excellent security enhancement with support for XHR/API token delivery.The implementation properly supports CSRF tokens from both POST fields and HTTP headers, which is essential for binary/chunked uploads. Key improvements verified:
- Uses
hash_equals()for timing-attack safe comparison- Maintains backward compatibility by preferring POST over header
- Properly cleans up both token sources after validation
- Both delivery methods are actively used in the codebase:
- XHR/header usage:
Browse.pagesetsX-CSRF-Tokenheader- POST usage: Multiple form implementations (DisplaySettings, UserEdit, UserAdd) submit tokens as hidden fields
emhttp/plugins/dynamix/styles/default-base.css (1)
94-118: LGTM - New CSS classes for UI enhancements.The
.small-caps-labeland.clipboard-tooltipclasses are well-structured and consistent with existing styling patterns in the file. The fixed positioning with centering transform for the tooltip is appropriate for clipboard feedback.emhttp/plugins/dynamix/include/Templates.php (2)
25-26: LGTM - Consistent template markup update.The change from
div.dfm_infotospan.dfm_textwith inline warning icons is applied consistently across all dialog templates. This addresses the Markdown parser regression mentioned in the PR objectives.
305-326: Permission select controls width update.The class change from
narrow dfmtodfmaddresses the PR objective to increase the width of permission selection controls. The change is applied consistently to Owner, Group, and Other selects.emhttp/plugins/dynamix/nchan/file_manager (4)
132-145: LGTM - Static state reset mechanism for transfer tracking.The reset parameter correctly clears all static variables (
$last_rsync_eta_seconds,$total_size,$total_calculations,$last_calc_percent) when starting a new transfer. This ensures clean state between operations.
167-199: Well-designed total size estimation with averaging.The multi-sample averaging approach (5 measurements at different progress percentages) effectively reduces the ~2% error per percent point caused by rsync's truncation behavior. The +0.5% adjustment is a reasonable heuristic to center the estimate.
344-348: JSON format migration for job parameters.Clean migration from INI to JSON format. The validation with
is_array($data)beforeextract()prevents issues with malformed JSON. This improves handling of special characters in filenames.
410-410: Correct placement of static state reset.The
parse_rsync_progress(null, null, true)call before starting copy operations ensures the total size estimation starts fresh for each transfer.emhttp/plugins/dynamix/include/PopularDestinations.php (1)
52-116: LGTM - Well-designed frequency-based scoring system.The scoring algorithm correctly:
- Finds the target path before decay
- Decays all non-target entries by 1
- Increments target by 10 (or creates it)
- Prunes non-positive scores
- Sorts and truncates
The
unset($dest)after the foreach reference loop is correctly placed to avoid reference issues.emhttp/plugins/dynamix/include/OpenTerminal.php (2)
54-90: Path-aware terminal launching implementation.The implementation correctly:
- Validates the path with
realpath()and falls back to/root- Properly escapes single quotes in the path
- Creates a startup script to change directory before launching bash
One consideration: The startup script at
/var/tmp/$name.run.shpersists after use. This is likely acceptable since the directory is tmpfs and the file is small.
77-78: The sed pattern is correct and should work on default Unraid systems.The default Unraid
/etc/profilecontains thecd $HOMEline that this pattern targets. The code itself documents this behavior (line 73 comment explicitly mentions it), and web search confirms the pattern exists in standard Unraid installations. No changes needed.Likely an incorrect or invalid review comment.
emhttp/plugins/dynamix/include/FileTree.php (1)
74-90: LGTM - FUSE conflict prevention logic.The filtering correctly prevents showing conflicting paths:
- In
/mnt/usercontext: only shows/mnt/userpaths or external mounts- In
/mnt/diskXcontext: excludes/mnt/userand/mnt/rootsharepathsThis prevents users from accidentally accessing the same data via both user shares and direct disk paths.
emhttp/plugins/dynamix/include/Browse.php (3)
85-86: LGTM - Broken symlink icon class.Appropriate visual indicator using
fa-chain-brokenwith red text for broken symlinks.
168-179: Robust directory listing with NULL separation.The new find command approach:
- Uses NULL-separated fields to handle filenames with newlines
- Separates working symlinks from broken symlinks with two find passes
- Includes symlink target information (
%l)The heredoc syntax with
BASHidentifier is clean.
188-225: LGTM - Device name extraction for symlinks.The logic correctly determines the device/location for both:
- Absolute symlinks: Uses the target path to find the actual storage location
- Relative symlinks/regular files: Uses the source path
This ensures the LOCATION column accurately reflects where the data physically resides.
emhttp/plugins/dynamix/include/Control.php (3)
46-46: LGTM - Flexible mode parameter handling.Using null-coalescing to check POST first, then GET, provides flexibility for different request types.
78-88: Raw binary upload with size validation.The new raw binary upload method reads directly from
php://inputwith a ~21MB limit. This is more efficient than base64 encoding (which has ~33% overhead).
222-226: LGTM - Popular destinations integration.Correctly updates popular destinations when copy/move operations (actions 3, 4, 8, 9) are queued or started, and the target is non-empty. Based on learnings, CSRF validation is handled globally.
emhttp/plugins/dynamix/Browse.page (9)
60-66: LGTM!The implementation correctly prevents the fileTree dropdown from closing by stopping mousedown event propagation. The namespaced event handler (
.dfmFileTree) allows for proper cleanup.
399-403: LGTM!The capture-phase event listeners are correctly implemented with proper cleanup on dialog close. This ensures the custom handlers run before the jQueryFileTree handlers and prevents memory leaks.
Also applies to: 525-533
536-644: LGTM with minor cleanup needed.The tree navigation functions are well-structured and handle asynchronous folder expansion appropriately with timeouts. The recursive approach in
openFolderRecursive()correctly handles the async nature of tree loading.Note: Console.log statements in
navigateFileTree()should be removed as flagged in the earlier comment.
698-698: LGTM!The increased dialog height (800px) for copy/move operations provides adequate space for the file tree navigation feature. This is consistent across all relevant actions (3, 4, 8, 9) and aligns with the PR's UI/UX improvements.
Also applies to: 708-708, 727-727, 735-735, 998-998, 1010-1010
785-787: LGTM!The dialog lifecycle management is correctly implemented:
closehandler removes the namespaced mousedown event to prevent memory leakspreventFileTreeClose()is called immediately after dialog creationsetupTargetNavigation()is conditionally invoked only for copy/move operations (actions 3, 4, 8, 9)Also applies to: 898-903, 1062-1064, 1178-1183
323-323: LGTM!The job queue iteration correctly extracts the queue index from element IDs. Per the PR objectives, this change corrects an off-by-one error in job queue numbering.
1189-1205: LGTM!The enhanced error handling provides better user feedback by categorizing upload failures (timeout, network, HTTP errors) and displaying appropriate localized messages. The addition of
currentXhr = nullensures proper cleanup.
1217-1289: LGTM!The rewrite to XMLHttpRequest provides significant improvements:
- Raw binary uploads eliminate Base64 overhead
- 20MB chunks improve upload performance
- Comprehensive error handling covers network failures, timeouts, and HTTP errors
- Proper abort handling allows user cancellation
- CSRF token protection is correctly implemented
The timeout calculation (1 minute per MB, minimum 10 minutes) is appropriate for various network conditions.
1291-1305: LGTM!The addition of
currentXhrenables proper upload cancellation. The cancel flow correctly:
- Sets the cancel flag
- Aborts the active XHR request
- Triggers the abort handler which performs cleanup
mgutt
force-pushed
the
fix-issue-2500-clean
branch
from
557dc5b to
2470a64
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
emhttp/plugins/dynamix/include/FileTree.php (1)
71-71: Undefined variable$autocomplete.The variable
$autocompleteis used on line 71 but is not defined anywhere in this file. This will cause a PHP notice/warning and the condition will always evaluate to true (since undefined evaluates to falsy).🔎 Proposed fix
Add the variable extraction near line 61:
$match = $_POST['match']; $checkbox = $_POST['multiSelect'] == 'true' ? "<input type='checkbox'>" : ""; +$autocomplete = ($_POST['autocomplete'] ?? '') === 'true';emhttp/plugins/dynamix/Browse.page (2)
367-391: Remove debugging console.log statements before production.Multiple
console.logstatements remain throughoutsetupTargetNavigation()and related functions. These should be removed for production code as they create console noise and can impact performance.
1337-1394: Add user feedback or fallback for clipboard functionality on HTTP-only deployments.The Clipboard API requires HTTPS. While the code gracefully handles failures with
.catch(), users on HTTP-only deployments receive no indication that clipboard functionality is unavailable. Consider implementing a fallback usingdocument.execCommand('copy').The current implementation silently fails on HTTP. Consider:
- Detecting
window.isSecureContext- Using
execCommand('copy')as fallback- Showing a brief message when clipboard is unavailable
🧹 Nitpick comments (8)
emhttp/plugins/dynamix/nchan/file_manager (2)
167-199: Consider extracting hardcoded thresholds as constants.The total size calculation logic is well-designed and handles rsync's percent truncation correctly. However, the hardcoded values (3% minimum progress at line 178, 5 samples at line 195) could be extracted as named constants for better maintainability and self-documentation.
🔎 Proposed refactor to use named constants
+// Constants for total size calculation +const MIN_PERCENT_FOR_TOTAL_CALC = 3; +const TOTAL_SIZE_SAMPLE_COUNT = 5; + function parse_rsync_progress($status, $action_label, $reset = false) { static $last_rsync_eta_seconds = null; static $total_size = null; static $total_calculations = []; static $last_calc_percent = null; if ($reset) { $last_rsync_eta_seconds = null; $total_size = null; $total_calculations = []; $last_calc_percent = null; return []; } // ... existing code ... - if ($total_size === null || count($total_calculations) < 5) { + if ($total_size === null || count($total_calculations) < TOTAL_SIZE_SAMPLE_COUNT) { $percent_val = intval(str_replace('%', '', $percent)); $is_running_line = !isset($parts[4]); - if ($is_running_line && $percent_val >= 3 && $last_calc_percent !== $percent_val) { + if ($is_running_line && $percent_val >= MIN_PERCENT_FOR_TOTAL_CALC && $last_calc_percent !== $percent_val) { // ... calculation logic ... - if (count($total_calculations) >= 5) { + if (count($total_calculations) >= TOTAL_SIZE_SAMPLE_COUNT) { $total_size = array_sum($total_calculations) / count($total_calculations); } } }
82-89: Consider extracting unit conversion into helper functions.The unit conversion logic appears in three places (lines 82-89, 180-187, and 222-229). While the current implementation works correctly, extracting this into reusable helper functions would improve maintainability and reduce the risk of inconsistencies.
Example helpers:
function size_string_to_bytes($size_str) { $multipliers = ['K' => 1024, 'M' => 1024*1024, 'G' => 1024*1024*1024, 'T' => 1024*1024*1024*1024]; $bytes = floatval($size_str); foreach ($multipliers as $unit => $mult) { if (stripos($size_str, $unit) !== false) { return $bytes * $mult; } } return $bytes; } function bytes_to_display_size($bytes) { $units = ['TB' => 1024**4, 'GB' => 1024**3, 'MB' => 1024**2, 'KB' => 1024]; foreach ($units as $unit => $divisor) { if ($bytes >= $divisor) { return number_format($bytes / $divisor, 2) . $unit; } } return number_format($bytes, 2) . 'B'; }Also applies to: 180-187, 222-229
emhttp/plugins/dynamix/include/Browse.php (1)
191-206: Potential issue with relative symlink target containing/.The condition
$target[0] == '/'only checks if the symlink target starts with/. A relative symlink like../disk2/foowould fall through to the else branch and extract device name from the source path, which is correct. However, consider adding a comment clarifying this behavior for maintainability.🔎 Suggested documentation improvement
if ($target && $target[0] == '/') { // Absolute symlink: extract device from target path // Example: /mnt/disk2/foo/bar -> dev[2] = 'disk2' $dev = explode('/', $target, 5); $dev_name = $dev[2] ?? ''; } else { - // Regular file/folder or relative symlink: extract from source path + // Regular file/folder or relative symlink: extract from source path + // Note: Relative symlinks (e.g., ../disk2/foo) use source path for device, + // which is intentional as the symlink resides on the source device // Example: /mnt/disk1/sharename/foo -> dev[3] = 'sharename', dev[2] = 'disk1' $dev = explode('/', $name, 5); $dev_name = $dev[3] ?? $dev[2];emhttp/plugins/dynamix/include/Control.php (2)
77-88: Consider validating chunk data before writing.When reading raw binary from
php://input, the code checks size but doesn't validate that data was actually received. Iffile_get_contents('php://input')fails or returns false, it would still attempt to write.🔎 Proposed fix
} else { // New raw binary upload method (read from request body) $chunk = file_get_contents('php://input'); + if ($chunk === false) { + unlink($local); + die('error:read'); + } if (strlen($chunk) > 21000000) { // slightly more than 20MB to allow overhead unlink($local); die('error:chunksize:'.strlen($chunk)); } }
137-146: Missing error handling for malformed JSON in jobs listing.If a job file contains invalid JSON,
json_decodereturnsnulland the code silently skips it withif (!$data) continue;. This is acceptable for resilience, but consider logging these cases for debugging.emhttp/plugins/dynamix/include/OpenTerminal.php (1)
74-82: Consider cleaning up temporary files after shell exits.The script creates
/tmp/$name.profilebut relies on thermcommand within the script. If the shell is killed unexpectedly, this file may remain. Consider using a trap or ensuring cleanup in the parent script.🔎 Suggested improvement
$script_content = <<<BASH #!/bin/bash +trap 'rm -f /tmp/$name.profile' EXIT # Modify /etc/profile to replace 'cd \$HOME' with our target path sed 's#^cd \$HOME#cd '\''$escaped_path'\''#' /etc/profile > /tmp/$name.profile source /tmp/$name.profile source /root/.bash_profile 2>/dev/null -rm /tmp/$name.profile exec bash --norc -i BASH;emhttp/plugins/dynamix/Browse.page (2)
406-418: Potential memory leak with document-level event listener.The
treeClickHandleris attached todocumentin capture phase but cleanup indialogclosemay not fire if the dialog is destroyed without proper close. Consider also cleaning up in the dialog'scloseoption.🔎 Proposed improvement
The cleanup is already handled in
dialogcloseevent (lines 525-532), but consider adding defensive cleanup:// Cleanup on dialog close dfm.window.on('dialogclose', function() { document.removeEventListener('click', treeClickHandler, true); document.removeEventListener('click', popularClickHandler, true); if (inputElement) { inputElement.removeEventListener('mousedown', preventClose, true); inputElement.removeEventListener('focus', preventClose, true); inputElement.removeEventListener('click', preventClose, true); } }); + + // Also cleanup on destroy (in case close doesn't fire) + dfm.window.on('dialogdestroy', function() { + document.removeEventListener('click', treeClickHandler, true); + document.removeEventListener('click', popularClickHandler, true); + });
614-618: Timeout duration may be insufficient for deep paths.The timeout
parts.length * 300 + 500assumes each folder takes 300ms to expand. For slow network/disk or deeply nested paths, this may be insufficient.Consider using a completion callback or promise-based approach instead of fixed timeouts for more reliable navigation completion detection.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
emhttp/plugins/dynamix/Browse.pageemhttp/plugins/dynamix/include/Browse.phpemhttp/plugins/dynamix/include/Control.phpemhttp/plugins/dynamix/include/FileTree.phpemhttp/plugins/dynamix/include/OpenTerminal.phpemhttp/plugins/dynamix/include/PopularDestinations.phpemhttp/plugins/dynamix/include/Templates.phpemhttp/plugins/dynamix/nchan/file_manageremhttp/plugins/dynamix/sheets/BrowseButton.cssemhttp/plugins/dynamix/styles/default-base.cssemhttp/plugins/dynamix/styles/default-dynamix.cssetc/rc.d/rc.nginx
💤 Files with no reviewable changes (2)
- emhttp/plugins/dynamix/sheets/BrowseButton.css
- emhttp/plugins/dynamix/styles/default-dynamix.css
🚧 Files skipped from review as they are similar to previous changes (3)
- emhttp/plugins/dynamix/styles/default-base.css
- etc/rc.d/rc.nginx
- emhttp/plugins/dynamix/include/PopularDestinations.php
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
Repo: unraid/webgui 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.
Applied to files:
emhttp/plugins/dynamix/include/FileTree.php
📚 Learning: 2025-10-04T05:22:33.141Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2421
File: emhttp/plugins/dynamix/include/DefaultPageLayout/MainContentTabless.php:23-0
Timestamp: 2025-10-04T05:22:33.141Z
Learning: In the Unraid webgui repository, `emhttp/webGui` is a symlink that points to `plugins/dynamix`. Therefore, paths using `$docroot/webGui/...` correctly resolve to `$docroot/plugins/dynamix/...` at runtime.
Applied to files:
emhttp/plugins/dynamix/include/FileTree.phpemhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-03-27T22:04:34.550Z
Learnt from: zackspear
Repo: unraid/webgui 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.
Applied to files:
emhttp/plugins/dynamix/include/FileTree.phpemhttp/plugins/dynamix/include/Control.php
📚 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/include/FileTree.phpemhttp/plugins/dynamix/include/Templates.phpemhttp/plugins/dynamix/include/Browse.phpemhttp/plugins/dynamix/include/OpenTerminal.phpemhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-06-21T00:10:40.789Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2258
File: emhttp/plugins/dynamix/DashStats.page:0-0
Timestamp: 2025-06-21T00:10:40.789Z
Learning: In the Unraid webgui codebase (emhttp/plugins/dynamix), replacing `<i>` elements with `<button>` elements for accessibility would require extensive CSS refactoring due to legacy CSS having direct button styles that would conflict with icon-based toggles.
Applied to files:
emhttp/plugins/dynamix/include/Templates.php
📚 Learning: 2025-09-05T19:26:36.587Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2354
File: emhttp/plugins/dynamix/ShareEdit.page:0-0
Timestamp: 2025-09-05T19:26:36.587Z
Learning: In emhttp/plugins/dynamix/ShareEdit.page, the clone-settings div was moved outside the form element and both are wrapped in a div.relative container to prevent event bubbling issues while maintaining proper positioning.
Applied to files:
emhttp/plugins/dynamix/include/Templates.phpemhttp/plugins/dynamix/Browse.page
📚 Learning: 2025-06-03T21:27:15.912Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2230
File: emhttp/plugins/dynamix/include/Templates.php:63-74
Timestamp: 2025-06-03T21:27:15.912Z
Learning: In the Unraid WebGUI codebase (emhttp/plugins/dynamix/include/Templates.php), there are known duplicate ID issues in checkbox templates across multiple template instances that the maintainers are aware of but have chosen not to address due to the effort required for legacy code improvements.
Applied to files:
emhttp/plugins/dynamix/include/Templates.php
🧬 Code graph analysis (3)
emhttp/plugins/dynamix/include/FileTree.php (1)
emhttp/plugins/dynamix/include/PopularDestinations.php (1)
getPopularDestinations(123-138)
emhttp/plugins/dynamix/include/Templates.php (1)
emhttp/plugins/dynamix/include/Helpers.php (1)
mk_option(182-184)
emhttp/plugins/dynamix/include/Control.php (1)
emhttp/plugins/dynamix/include/PopularDestinations.php (1)
updatePopularDestinations(52-116)
🔇 Additional comments (19)
emhttp/plugins/dynamix/nchan/file_manager (2)
211-235: LGTM! Clean progress display with proper localization.The progress text formatting properly localizes all user-facing strings and gracefully handles the edge case where total size hasn't been calculated yet. The unit conversion and display formatting are appropriate.
410-410: Excellent localization improvements!The explicit reset calls before copy/move operations ensure clean state management, and wrapping the error messages with
_()makes them properly localizable. This directly addresses one of the objectives from issue #2500.Also applies to: 420-420, 454-454, 521-521, 551-551
emhttp/plugins/dynamix/include/Browse.php (4)
85-86: LGTM!Good addition of the broken-symlink icon case with appropriate red styling to visually indicate broken links.
154-161: LGTM!The octal escape decoding correctly handles special characters (like newlines
\012) thatgetfattroutputs, ensuring proper filename matching with thefindresults.
168-179: LGTM!The two-phase
findapproach correctly separates working symlinks (! -xtype l) from broken symlinks (-xtype l), using NULL-separation to handle filenames with newlines. The 7-field format is well-documented.
242-247: LGTM!Broken symlink detection correctly maps to the special icon class and disables the
onclickhandler for broken items, preventing users from attempting to edit non-existent targets.emhttp/plugins/dynamix/include/Control.php (3)
164-177: LGTM!The start mode correctly reads the first JSON line, shifts it to active, and handles cleanup of the jobs file when empty.
185-199: Potential issue with array re-indexing after unset.After
unset($lines[$line_number]), the array has gaps. When writing back withimplode("\n", $lines), this works correctly, but if other code expects contiguous indices, it could cause issues. The current implementation is correct since you're just joining lines.
208-235: LGTM!The file mode correctly builds structured JSON data, triggers
updatePopularDestinations()for copy/move actions (3, 4, 8, 9), and handles both queue and immediate start flows consistently.emhttp/plugins/dynamix/include/Templates.php (2)
25-26: LGTM!The template changes consistently replace
<div class="dfm_info">with<span class="dfm_text">across all action dialogs, addressing the Markdown parser regression mentioned in issue #2500. The structure is now uniform.Also applies to: 36-37, 53-54, 76-78, 103-105, 118-119, 135-136, 158-160, 185-187, 200-201, 213-218, 240-242, 267-269, 290-292, 331-333
305-326: LGTM!Removing the
narrowclass from the permission select elements addresses the usability request in issue #2500 to increase the width of permission selection controls.emhttp/plugins/dynamix/include/OpenTerminal.php (1)
62-66: LGTM!Good defensive coding - falling back to
/rootwhen the requested path doesn't exist prevents errors while still providing terminal access.emhttp/plugins/dynamix/include/FileTree.php (3)
74-90: LGTM!The FUSE conflict prevention logic correctly filters popular destinations based on context:
- In
/mnt/usercontext: only shows/mnt/userpaths or external mounts- In
/mnt/diskXcontext: excludes/mnt/userand/mnt/rootsharepathsThis prevents users from accidentally creating cross-FUSE operations.
93-106: LGTM!Popular destinations are rendered with appropriate styling and use
data-pathinstead ofrelto prevent jQueryFileTree from handling these links directly, allowing custom click handling in Browse.page.
109-156: LGTM!The refactored directory reading logic correctly populates
$dirsand$filesarrays before rendering, supporting both normal and autocomplete modes with consistent behavior.emhttp/plugins/dynamix/Browse.page (4)
60-66: LGTM!The
preventFileTreeClose()function correctly uses a namespaced event handler (mousedown.dfmFileTree) that can be properly cleaned up on dialog close, preventing memory leaks.
1217-1230: LGTM on XHR upload implementation!The switch from jQuery AJAX to native XHR with:
- Raw binary uploads (no base64 overhead)
- Proper CSRF token header
- Configurable timeout based on chunk size
- Abort capability via
currentXhrThis is a significant improvement for upload reliability and performance.
1265-1276: Good abort handling with cleanup.The
onaborthandler correctly triggers server-side cleanup via the cancel parameter, ensuring partial uploads are removed. The.always()ensures UI cleanup regardless of POST result.
1351-1358: Good UX: Terminal button in path dialog.The "Terminal" button (using the cancel button slot) provides quick access to open a terminal at the displayed path, addressing the "Open Terminal here" feature request from issue #2500.
mgutt
force-pushed
the
fix-issue-2500-clean
branch
from
2470a64 to
2505068
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
emhttp/plugins/dynamix/nchan/file_manager (1)
344-348: Consider adding error logging for JSON parse failures.While the code safely handles JSON decode failures via the
is_array()check, malformed JSON will silently fail. For a long-running daemon, this could make debugging difficult.🔎 Proposed enhancement
if (file_exists($active)) { $data = json_decode(file_get_contents($active), true); - if (is_array($data)) extract($data); + if (is_array($data)) { + extract($data); + } elseif ($data === null && json_last_error() !== JSON_ERROR_NONE) { + error_log("File Manager: Failed to parse JSON from $active: " . json_last_error_msg()); + } }emhttp/plugins/dynamix/Browse.page (4)
1282-1339: Add fallback for clipboard functionality on non-HTTPS deployments.The Clipboard API requires a secure context (HTTPS). While the
.catch()handles API failures silently, users on HTTP-only deployments receive no indication that clipboard copying failed. Consider adding a fallback usingdocument.execCommand('copy')or displaying a message.
354-534: Remove debugging console.log statements before merging.The
setupTargetNavigation()function contains numerousconsole.logstatements (lines 367, 372, 376-377, 379, 382, 387, 390, 465, 470, 484, 517, 519, etc.) that should be removed for production code.
556-579: Remove console.log statements from resetFileTree.Lines 557, 562, 571, 577 contain debugging statements that should be removed.
581-619: Remove console.log statements from navigateFileTree and openFolderRecursive.Lines 582, 585, 593, 596, 602, 605-609, 615 contain debugging statements.
emhttp/plugins/dynamix/include/OpenTerminal.php (1)
70-82: Escape sed metacharacters in the replacement string.The
str_replace("'", "'\\''", $real_path)correctly handles single quotes for shell interpretation, but sed replacement string special characters (&and\) are not escaped. In sed,&in the replacement string expands to the matched text, which will corrupt paths containing&. Consider escaping sed metacharacters:🔎 Proposed fix
- $escaped_path = str_replace("'", "'\\''", $real_path); + // Escape for sed replacement: & and \ are special + $sed_escaped = str_replace(['\\', '&'], ['\\\\', '\\&'], $real_path); + // Then escape for shell single quotes + $escaped_path = str_replace("'", "'\\''", $sed_escaped);emhttp/plugins/dynamix/include/FileTree.php (1)
71-71: Undefined variable$autocomplete.The variable
$autocompleteis used but not extracted from POST data. This will cause a PHP notice and the condition will always evaluate to false (treating undefined as falsy).🔎 Proposed fix
Add near line 61:
$match = $_POST['match']; $checkbox = $_POST['multiSelect'] == 'true' ? "<input type='checkbox'>" : ""; +$autocomplete = ($_POST['autocomplete'] ?? '') === 'true';
🧹 Nitpick comments (3)
emhttp/plugins/dynamix/include/OpenTerminal.php (1)
56-66: Consider validating the path is under allowed roots.The code validates that the path exists via
realpath(), but doesn't verify it's under an allowed root (e.g.,/mnt/or/boot/). A user could potentially open a terminal at any readable system directory. Consider adding root validation consistent with other file manager operations.🔎 Proposed enhancement
// Validate path $real_path = realpath($more); - if ($real_path === false) { + if ($real_path === false || !preg_match('#^/(mnt|boot)/#', $real_path)) { // Path doesn't exist - fall back to home directory $real_path = '/root'; }emhttp/plugins/dynamix/include/Control.php (1)
184-211: Verify action type comparison uses consistent types.At lines 200 and 208, actions are compared as strings (
'3', '4', '8', '9'), but$_POST['action']may arrive as a string from the form. This should work correctly, but consider usingin_array((int)$data['action'], [3, 4, 8, 9])for explicit type handling.🔎 Proposed fix for type consistency
- if (in_array($data['action'], ['3', '4', '8', '9']) && !empty($data['target'])) { + if (in_array((int)$data['action'], [3, 4, 8, 9]) && !empty($data['target'])) { updatePopularDestinations($data['target']); }emhttp/plugins/dynamix/include/PopularDestinations.php (1)
43-46: Consider adding error handling for file write operations.
file_put_contents()can fail (permissions, disk full, etc.) but the function doesn't check the return value or handle errors. While failures are unlikely on/boot/config/, adding basic error handling would improve robustness.🔎 Proposed enhancement
function savePopularDestinations($data) { $json = json_encode($data, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES); - file_put_contents(POPULAR_DESTINATIONS_FILE, $json); + $result = file_put_contents(POPULAR_DESTINATIONS_FILE, $json, LOCK_EX); + if ($result === false) { + error_log("PopularDestinations: Failed to write to " . POPULAR_DESTINATIONS_FILE); + } }Note:
LOCK_EXalso provides basic protection against concurrent writes.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
emhttp/plugins/dynamix/Browse.pageemhttp/plugins/dynamix/include/Browse.phpemhttp/plugins/dynamix/include/Control.phpemhttp/plugins/dynamix/include/FileTree.phpemhttp/plugins/dynamix/include/OpenTerminal.phpemhttp/plugins/dynamix/include/PopularDestinations.phpemhttp/plugins/dynamix/include/Templates.phpemhttp/plugins/dynamix/nchan/file_manageremhttp/plugins/dynamix/sheets/BrowseButton.cssemhttp/plugins/dynamix/styles/default-base.cssemhttp/plugins/dynamix/styles/default-dynamix.css
💤 Files with no reviewable changes (2)
- emhttp/plugins/dynamix/sheets/BrowseButton.css
- emhttp/plugins/dynamix/styles/default-dynamix.css
🚧 Files skipped from review as they are similar to previous changes (1)
- emhttp/plugins/dynamix/styles/default-base.css
🧰 Additional context used
🧠 Learnings (9)
📚 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/include/PopularDestinations.phpemhttp/plugins/dynamix/include/OpenTerminal.phpemhttp/plugins/dynamix/include/Templates.phpemhttp/plugins/dynamix/include/Browse.phpemhttp/plugins/dynamix/include/Control.phpemhttp/plugins/dynamix/include/FileTree.php
📚 Learning: 2025-06-21T00:10:40.789Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2258
File: emhttp/plugins/dynamix/DashStats.page:0-0
Timestamp: 2025-06-21T00:10:40.789Z
Learning: In the Unraid webgui codebase (emhttp/plugins/dynamix), replacing `<i>` elements with `<button>` elements for accessibility would require extensive CSS refactoring due to legacy CSS having direct button styles that would conflict with icon-based toggles.
Applied to files:
emhttp/plugins/dynamix/include/Templates.php
📚 Learning: 2025-09-05T19:26:36.587Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2354
File: emhttp/plugins/dynamix/ShareEdit.page:0-0
Timestamp: 2025-09-05T19:26:36.587Z
Learning: In emhttp/plugins/dynamix/ShareEdit.page, the clone-settings div was moved outside the form element and both are wrapped in a div.relative container to prevent event bubbling issues while maintaining proper positioning.
Applied to files:
emhttp/plugins/dynamix/include/Templates.phpemhttp/plugins/dynamix/Browse.page
📚 Learning: 2025-06-03T21:27:15.912Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2230
File: emhttp/plugins/dynamix/include/Templates.php:63-74
Timestamp: 2025-06-03T21:27:15.912Z
Learning: In the Unraid WebGUI codebase (emhttp/plugins/dynamix/include/Templates.php), there are known duplicate ID issues in checkbox templates across multiple template instances that the maintainers are aware of but have chosen not to address due to the effort required for legacy code improvements.
Applied to files:
emhttp/plugins/dynamix/include/Templates.php
📚 Learning: 2025-10-04T05:22:33.141Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2421
File: emhttp/plugins/dynamix/include/DefaultPageLayout/MainContentTabless.php:23-0
Timestamp: 2025-10-04T05:22:33.141Z
Learning: In the Unraid webgui repository, `emhttp/webGui` is a symlink that points to `plugins/dynamix`. Therefore, paths using `$docroot/webGui/...` correctly resolve to `$docroot/plugins/dynamix/...` at runtime.
Applied to files:
emhttp/plugins/dynamix/include/Control.phpemhttp/plugins/dynamix/include/FileTree.php
📚 Learning: 2025-03-27T22:04:34.550Z
Learnt from: zackspear
Repo: unraid/webgui 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.
Applied to files:
emhttp/plugins/dynamix/include/Control.phpemhttp/plugins/dynamix/include/FileTree.php
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
Repo: unraid/webgui 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.
Applied to files:
emhttp/plugins/dynamix/include/FileTree.phpemhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-02-27T21:52:42.541Z
Learnt from: dlandon
Repo: unraid/webgui PR: 2035
File: emhttp/plugins/dynamix/SyslogFilter.page:19-26
Timestamp: 2025-02-27T21:52:42.541Z
Learning: In the unraid/webgui repository, basic error handling is sufficient for file operations like checking file existence and using the @ operator to suppress errors, without additional error messages or notifications.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-02-27T21:53:37.589Z
Learnt from: dlandon
Repo: unraid/webgui PR: 2035
File: etc/rc.d/rc.rsyslogd:48-50
Timestamp: 2025-02-27T21:53:37.589Z
Learning: For the unraid/webgui project, simple error handling is preferred over extensive error checking in shell scripts. The copy operation for configuration files like in rc.rsyslogd doesn't require additional error logging, permission checks, or return statements.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
🧬 Code graph analysis (4)
emhttp/plugins/dynamix/include/Templates.php (1)
emhttp/plugins/dynamix/include/Helpers.php (1)
mk_option(182-184)
emhttp/plugins/dynamix/include/Browse.php (4)
emhttp/plugins/dynamix/include/SmartInfo.php (1)
size(30-32)emhttp/plugins/dynamix/include/FileTree.php (1)
path(30-32)emhttp/plugins/dynamix/include/Helpers.php (1)
my_scale(19-39)emhttp/plugins/dynamix/include/Control.php (1)
my_age(26-36)
emhttp/plugins/dynamix/include/Control.php (1)
emhttp/plugins/dynamix/include/PopularDestinations.php (1)
updatePopularDestinations(52-116)
emhttp/plugins/dynamix/include/FileTree.php (1)
emhttp/plugins/dynamix/include/PopularDestinations.php (1)
getPopularDestinations(123-138)
🔇 Additional comments (22)
emhttp/plugins/dynamix/include/Templates.php (1)
25-26: LGTM - Consistent UI template updates.The changes uniformly switch from
<div class="dfm_info">to inline<span class="dfm_text">elements for warning notices, and simplify permission select controls by removing thenarrowclass. This aligns with the broader UI simplification effort in this PR.Also applies to: 36-37, 53-54, 76-78, 103-105, 117-119, 134-136, 158-160, 185-187, 199-201, 214-218, 240-242, 267-269, 290-292, 305-333
emhttp/plugins/dynamix/nchan/file_manager (3)
132-145: Good implementation of reset mechanism for static state.The
$resetparameter properly clears all static variables and returns an empty array, ensuring clean state between separate transfer operations. The calls at lines 410 and 454 correctly reset before starting new copy/move actions.
167-199: Total size estimation logic is well-designed.The multi-sample averaging approach (5 measurements at different percentages) accounts for rsync's percentage truncation and provides approximately 2% accuracy as documented. The +0.5% adjustment at line 190 is a reasonable heuristic for truncation compensation.
420-421: Good use of localization for error messages.The error messages are now properly wrapped with
_()for translation support, addressing one of the linked issue objectives.Also applies to: 521-522, 551-552
emhttp/plugins/dynamix/Browse.page (3)
60-66: Good approach for preventing fileTree closure within dialogs.The
preventFileTreeClose()function correctly uses event namespacing (.dfmFileTree) for proper cleanup and stops propagation to prevent the document-level handler from closing the tree.
785-787: Good cleanup of fileTree event handlers on dialog close.The dialog close handlers properly detach the
.dfmFileTreenamespaced events, preventing memory leaks and stale handlers.Also applies to: 1062-1064
698-699: Dialog height increase accommodates new navigation UI.The height increase from 630 to 800 for copy/move dialogs provides adequate space for the enhanced file tree navigation features.
Also applies to: 708-709, 727-728, 735-736, 998-999, 1010-1011
emhttp/plugins/dynamix/include/Control.php (3)
113-122: Good migration to JSON format for jobs queue.The JSON line-based approach is cleaner than INI parsing and handles complex data structures better. The empty/invalid line guards at lines 114 and 116 provide robust error handling.
140-154: Correct implementation of start operation with JSON.Reading the first line as JSON, writing to active, and shifting the array properly handles the queue. The cleanup when empty (line 150) is correct.
161-174: Index conversion handles 1-based to 0-based correctly.The conversion at line 163 (
$row - 1) properly maps user-facing job numbers to array indices.emhttp/plugins/dynamix/include/FileTree.php (3)
74-90: Good FUSE conflict prevention logic.The filtering correctly prevents showing
/mnt/userpaths when in disk context and vice versa, avoiding FUSE layer conflicts that could cause issues.
92-106: Popular destinations UI rendering is well-structured.The HTML structure with appropriate classes (
popular-header,popular-destination,popular-separator) and the use ofdata-pathinstead ofrelto prevent jQueryFileTree interference is a good approach.
109-130: Directory content reading refactored for clarity.Pre-populating
$dirsand$filesarrays before rendering improves code organization and makes the logic easier to follow.emhttp/plugins/dynamix/include/Browse.php (5)
85-87: Good addition of broken symlink icon support.The new
broken-symlinkcase withfa-chain-broken red-textprovides clear visual indication of broken symlinks.
154-161: Correct octal escape decoding for getfattr output.The
preg_replace_callbackproperly decodes octal escapes (like\012for newline) that getfattr outputs for special characters, ensuring accurate filename matching.
165-179: Robust NULL-separated find output for filenames with newlines.Using
\0as a field separator instead of newlines correctly handles filenames containing newline characters. The two-find approach (working symlinks vs broken symlinks) is elegant.
191-206: Correct device name derivation for symlinks.The logic properly distinguishes between absolute symlink targets (extracting device from target path) and regular files/relative symlinks (extracting from source path). This ensures the LOCATION column shows the actual storage location.
242-247: Good broken symlink detection and handling.Broken symlinks are correctly identified, assigned the
broken-symlinkextension for icon mapping, and have theironclickdisabled to prevent attempting to edit non-existent targets.emhttp/plugins/dynamix/include/PopularDestinations.php (4)
17-20: Well-defined configuration constants.The constants provide clear documentation of the scoring system parameters and make tuning straightforward.
25-38: Robust load function with proper fallbacks.The function handles missing files and malformed JSON gracefully by returning an empty destinations array.
52-116: Scoring algorithm is well-designed.The decay-on-update approach naturally ages out unused destinations while boosting frequently used ones. The implementation correctly:
- Skips invalid paths (empty,
/mnt,/boot)- Normalizes trailing slashes
- Finds target before decay to avoid double-processing
- Prunes zero/negative scores
- Maintains sorted order and respects
MAX_ENTRIES
123-138: Retrieval function is correct and defensive.Re-sorting before slicing ensures correctness even if the stored data is somehow out of order. Returning only the path strings (not full objects) is appropriate for the UI consumer.
mgutt
force-pushed
the
fix-issue-2500-clean
branch
from
2505068 to
bc0ea9a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
emhttp/plugins/dynamix/include/FileTree.php (1)
71-71: Undefined variable$autocomplete.The variable
$autocompleteis used but never defined in this file. It should be extracted from POST data near line 61.🔎 Proposed fix
$match = $_POST['match']; $checkbox = $_POST['multiSelect'] == 'true' ? "<input type='checkbox'>" : ""; +$autocomplete = ($_POST['autocomplete'] ?? '') === 'true';emhttp/plugins/dynamix/Browse.page (1)
354-534: Remove debugging console.log statements before production.This function contains numerous
console.logstatements that should be removed for production code. These create unnecessary console noise and can impact performance.
🧹 Nitpick comments (6)
emhttp/plugins/dynamix/nchan/file_manager (3)
167-199: Extract duplicated byte-conversion logic.The multipliers array and byte conversion logic (lines 180-187) is duplicated from the
calculate_etafunction (lines 82-89). Extract this into a shared helper function to improve maintainability.🔎 Suggested refactor
Add a helper function before
calculate_eta:function convert_size_to_bytes($size_str) { $multipliers = ['K' => 1024, 'M' => 1024*1024, 'G' => 1024*1024*1024, 'T' => 1024*1024*1024*1024]; $bytes = floatval($size_str); foreach ($multipliers as $unit => $mult) { if (stripos($size_str, $unit) !== false) { $bytes *= $mult; break; } } return $bytes; }Then replace lines 180-187 with:
- // Convert transferred size to bytes - $multipliers = ['K' => 1024, 'M' => 1024*1024, 'G' => 1024*1024*1024, 'T' => 1024*1024*1024*1024]; - $transferred_bytes = floatval($transferred); - foreach ($multipliers as $unit => $mult) { - if (stripos($transferred, $unit) !== false) { - $transferred_bytes *= $mult; - break; - } - } + $transferred_bytes = convert_size_to_bytes($transferred);And similarly update
calculate_etato use the helper.
167-199: Document the total-size estimation constraints.The algorithm requires 5 samples at different percent values (≥3%) from running-transfer lines. For very small or fast transfers that complete before reaching 5 distinct percent values,
$total_sizeremains null and displays "N/A". Consider adding a comment explaining these constraints and the 0.5% truncation-adjustment heuristic.
410-410: Consider a dedicated reset method for clarity.The calls
parse_rsync_progress(null, null, true)correctly reset state but are not self-documenting. While functional, a dedicated static method or a clearer API (e.g.,reset_rsync_progress()) would improve code readability.💡 Alternative approach
Extract the reset logic:
function reset_rsync_progress() { parse_rsync_progress(null, null, true); }Then use:
- parse_rsync_progress(null, null, true); // Reset static variables + reset_rsync_progress(); // Reset static variables before new transferAlso applies to: 454-454
emhttp/plugins/dynamix/include/FileTree.php (1)
80-88:array_filterdoes not reindex arrays by default.While this doesn't cause an issue in the current
foreachusage, it may cause unexpected behavior if the array is later used with index-based access. Consider usingarray_values()to reindex after filtering.🔎 Proposed fix
if ($isUserContext) { // In /mnt/user context: only show /mnt/user paths OR non-/mnt paths (external mounts) - $popularPaths = array_filter($popularPaths, function($path) { + $popularPaths = array_values(array_filter($popularPaths, function($path) { return (strpos($path, '/mnt/user') === 0 || strpos($path, '/mnt/rootshare') === 0 || strpos($path, '/mnt/') !== 0); - }); + })); } else if (strpos($root, '/mnt/') === 0) { // In /mnt/diskX or /mnt/cache context: exclude /mnt/user and /mnt/rootshare paths - $popularPaths = array_filter($popularPaths, function($path) { + $popularPaths = array_values(array_filter($popularPaths, function($path) { return (strpos($path, '/mnt/user') !== 0 && strpos($path, '/mnt/rootshare') !== 0); - }); + })); }emhttp/plugins/dynamix/include/PopularDestinations.php (1)
52-59: Path validation could be bypassed with path traversal.While
/mntand/bootare explicitly blocked, paths containing..or other traversal sequences are not validated. Consider adding additional validation to ensure paths are within expected directories.🔎 Proposed fix
function updatePopularDestinations($targetPath) { // Skip empty paths or paths that are just /mnt or /boot if (empty($targetPath) || $targetPath == '/mnt' || $targetPath == '/boot') { return; } + // Validate path is within allowed directories + $realPath = realpath($targetPath); + if ($realPath === false || (strpos($realPath, '/mnt/') !== 0 && strpos($realPath, '/boot/') !== 0)) { + return; + } + // Normalize path (remove trailing slash) $targetPath = rtrim($targetPath, '/');emhttp/plugins/dynamix/Browse.page (1)
613-618: Timeout calculation for flag reset may be too short for deep paths.The formula
parts.length * 300 + 500gives 800ms for a path 1 level deep, but network latency or slow filesystem responses could cause the flag to reset before navigation completes, leading to input value restoration issues.Consider using a completion callback from
openFolderRecursiveinstead of a fixed timeout, or increase the base timeout.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
emhttp/plugins/dynamix/Browse.pageemhttp/plugins/dynamix/include/Browse.phpemhttp/plugins/dynamix/include/Control.phpemhttp/plugins/dynamix/include/FileTree.phpemhttp/plugins/dynamix/include/OpenTerminal.phpemhttp/plugins/dynamix/include/PopularDestinations.phpemhttp/plugins/dynamix/include/Templates.phpemhttp/plugins/dynamix/nchan/file_manageremhttp/plugins/dynamix/sheets/BrowseButton.cssemhttp/plugins/dynamix/styles/default-base.cssemhttp/plugins/dynamix/styles/default-dynamix.css
💤 Files with no reviewable changes (2)
- emhttp/plugins/dynamix/styles/default-dynamix.css
- emhttp/plugins/dynamix/sheets/BrowseButton.css
🚧 Files skipped from review as they are similar to previous changes (2)
- emhttp/plugins/dynamix/include/OpenTerminal.php
- emhttp/plugins/dynamix/include/Templates.php
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-02-27T21:52:42.541Z
Learnt from: dlandon
Repo: unraid/webgui PR: 2035
File: emhttp/plugins/dynamix/SyslogFilter.page:19-26
Timestamp: 2025-02-27T21:52:42.541Z
Learning: In the unraid/webgui repository, basic error handling is sufficient for file operations like checking file existence and using the @ operator to suppress errors, without additional error messages or notifications.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
Repo: unraid/webgui 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.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manageremhttp/plugins/dynamix/include/FileTree.php
📚 Learning: 2025-02-27T21:53:37.589Z
Learnt from: dlandon
Repo: unraid/webgui PR: 2035
File: etc/rc.d/rc.rsyslogd:48-50
Timestamp: 2025-02-27T21:53:37.589Z
Learning: For the unraid/webgui project, simple error handling is preferred over extensive error checking in shell scripts. The copy operation for configuration files like in rc.rsyslogd doesn't require additional error logging, permission checks, or return statements.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-10-04T05:22:33.141Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2421
File: emhttp/plugins/dynamix/include/DefaultPageLayout/MainContentTabless.php:23-0
Timestamp: 2025-10-04T05:22:33.141Z
Learning: In the Unraid webgui repository, `emhttp/webGui` is a symlink that points to `plugins/dynamix`. Therefore, paths using `$docroot/webGui/...` correctly resolve to `$docroot/plugins/dynamix/...` at runtime.
Applied to files:
emhttp/plugins/dynamix/include/FileTree.phpemhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-03-27T22:04:34.550Z
Learnt from: zackspear
Repo: unraid/webgui 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.
Applied to files:
emhttp/plugins/dynamix/include/FileTree.phpemhttp/plugins/dynamix/include/Control.php
📚 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/include/FileTree.phpemhttp/plugins/dynamix/include/PopularDestinations.phpemhttp/plugins/dynamix/include/Control.phpemhttp/plugins/dynamix/include/Browse.php
📚 Learning: 2025-06-21T00:10:40.789Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2258
File: emhttp/plugins/dynamix/DashStats.page:0-0
Timestamp: 2025-06-21T00:10:40.789Z
Learning: In the Unraid webgui codebase (emhttp/plugins/dynamix), replacing `<i>` elements with `<button>` elements for accessibility would require extensive CSS refactoring due to legacy CSS having direct button styles that would conflict with icon-based toggles.
Applied to files:
emhttp/plugins/dynamix/styles/default-base.css
📚 Learning: 2025-12-30T20:27:59.455Z
Learnt from: mgutt
Repo: unraid/webgui PR: 2501
File: emhttp/plugins/dynamix/Browse.page:1282-1339
Timestamp: 2025-12-30T20:27:59.455Z
Learning: In emhttp/plugins/dynamix/Browse.page, the xlink() function's clipboard functionality is intentionally designed to silently fail on HTTP connections without showing warnings. The path text is pre-selected to allow manual copying, which is preferred over showing repetitive error messages that would annoy HTTP users.
Applied to files:
emhttp/plugins/dynamix/Browse.page
📚 Learning: 2025-09-05T19:26:36.587Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2354
File: emhttp/plugins/dynamix/ShareEdit.page:0-0
Timestamp: 2025-09-05T19:26:36.587Z
Learning: In emhttp/plugins/dynamix/ShareEdit.page, the clone-settings div was moved outside the form element and both are wrapped in a div.relative container to prevent event bubbling issues while maintaining proper positioning.
Applied to files:
emhttp/plugins/dynamix/Browse.page
🧬 Code graph analysis (2)
emhttp/plugins/dynamix/include/FileTree.php (1)
emhttp/plugins/dynamix/include/PopularDestinations.php (1)
getPopularDestinations(123-138)
emhttp/plugins/dynamix/include/Control.php (1)
emhttp/plugins/dynamix/include/PopularDestinations.php (1)
updatePopularDestinations(52-116)
🔇 Additional comments (20)
emhttp/plugins/dynamix/nchan/file_manager (2)
211-235: LGTM!The progress display formatting correctly handles both calculated and unavailable total-size cases, with proper localization and unit conversion.
420-420: LGTM!Error messages are now properly wrapped for localization, making them translation-ready and consistent with the rest of the file's localized strings.
Also applies to: 521-521, 551-551
emhttp/plugins/dynamix/styles/default-base.css (1)
94-118: Unused CSS class and code duplication detected.The
.clipboard-tooltipclass (lines 101-118) is defined in the stylesheet but does not appear to be used anywhere in the codebase. If this is incomplete feature work, consider either implementing its usage or removing the unused CSS. The z-index and positioning concerns in the definition are moot if the class is not applied.Additionally,
.small-caps-label(lines 94-100) duplicates 5 properties from the existinga.info spanstyles (lines 73-89):white-space,font-variant,line-height,color. Consider extracting these shared properties into a reusable base class to reduce duplication and maintenance burden.⛔ Skipped due to learnings
Learnt from: zackspear Repo: unraid/webgui PR: 2258 File: emhttp/plugins/dynamix/DashStats.page:0-0 Timestamp: 2025-06-21T00:10:40.789Z Learning: In the Unraid webgui codebase (emhttp/plugins/dynamix), replacing `<i>` elements with `<button>` elements for accessibility would require extensive CSS refactoring due to legacy CSS having direct button styles that would conflict with icon-based toggles.emhttp/plugins/dynamix/include/FileTree.php (1)
93-106: Popular destinations rendering looks good.The implementation correctly:
- Adds a header with proper styling
- Truncates long names for display
- Uses
data-pathto avoid jQueryFileTree interference- Includes a visual separator
emhttp/plugins/dynamix/include/Browse.php (4)
85-86: Good addition of broken symlink visual indicator.The
fa-chain-broken red-texticon clearly indicates broken symlinks to users.
168-173: The two-find approach for symlink handling is well-designed.Using
-xtype lto detect broken symlinks separately from regular entries ensures accurate type detection. The NULL-separator approach correctly handles filenames with newlines.
182-186: Potential issue with incomplete entries at the end of output.If the find command output is truncated or malformed, the loop condition
$i + 7 <= count($fields_array)may miss this. Additionally, the last element afterexplode("\0", ...)will be an empty string due to the trailing\0, which is handled correctly by the loop condition.
242-247: Broken symlink handling correctly disables file editing.The conditional
onclickattribute prevents users from attempting to edit broken symlinks, which would fail. Thenl2br()usage preserves newlines in filenames for display.emhttp/plugins/dynamix/include/PopularDestinations.php (2)
77-85: Reference variable in foreach requires explicit unset.The
&$destreference is correctly unset on line 85. This prevents the common PHP pitfall where the reference remains bound after the loop.
123-138:getPopularDestinationsimplementation is clean and correct.The function correctly loads data, sorts by score, and returns only the paths. The defensive re-sort on line 128 is a good safeguard.
emhttp/plugins/dynamix/include/Control.php (4)
164-177: Start mode correctly handles the new JSON line-based queue.The logic properly:
- Reads the first line as the active job
- Removes it from the queue
- Cleans up empty queue files
- Returns appropriate state codes (0/1/2)
184-198: Off-by-one fix in undo mode is correct.The conversion from 1-based job numbers (user-facing) to 0-based array indices (
$row - 1) properly addresses the off-by-one issue mentioned in the PR objectives.
222-234: Popular destinations integration for copy/move operations.The code correctly updates popular destinations only for relevant actions (3=copy folder, 4=move folder, 8=copy file, 9=move file) and checks for non-empty targets. This is called in both direct start and queue paths.
78-88: Raw binary upload with size validation is a good security measure.The 21MB limit check prevents memory exhaustion from oversized chunks. The dual-method approach (legacy base64 vs new raw binary) maintains backward compatibility.
emhttp/plugins/dynamix/Browse.page (6)
60-66:preventFileTreeClosecorrectly prevents dialog dismissal.Using namespaced event binding (
mousedown.dfmFileTree) allows proper cleanup on dialog close (lines 786, 1063).
406-418: Document-level event listener attached in capture phase.The
treeClickHandleris attached todocumentin capture phase (line 418) and properly cleaned up on dialog close (line 526). This pattern is correct for intercepting events before they bubble.
1219-1230: Upload chunk size increased with appropriate timeout scaling.The 20MB raw binary chunks (vs previous base64-encoded) reduce overhead. The timeout calculation
Math.max(600000, slice / 1024 * 60)provides ~1 minute per MB with a 10-minute minimum, which is reasonable for large files on slow connections.
1265-1276: XHR abort handler properly triggers server-side cleanup.When the user cancels, the handler sends a cancel request to delete the partial upload, then cleans up the UI. Using
.always()ensures UI cleanup happens regardless of the cleanup POST's success.
1337-1394: Clipboard functionality intentionally silent-fails on HTTP.Based on learnings, this is the intended behavior - the path is pre-selected for manual copying on HTTP connections, and showing repetitive errors would degrade user experience.
698-735: Dialog height increases accommodate new navigation UI.The height changes from 630 to 800 for copy/move dialogs provide space for the enhanced file tree navigation features.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
emhttp/plugins/dynamix/include/Control.php (1)
164-177: Validate JSON before writing to active file.The first line from the jobs queue is written to the active file without verifying it's valid JSON. If a corrupted or legacy-format entry is first in queue, the backend will receive invalid data. Consider validating with
json_decode()before writing, consistent with the 'jobs' case handling.Suggested validation
$lines = file($jobs, FILE_IGNORE_NEW_LINES); if (!empty($lines)) { - file_put_contents($active, $lines[0]); + $firstJob = $lines[0]; + // Validate JSON before writing to active + if (json_decode($firstJob, true) !== null || $firstJob === 'null') { + file_put_contents($active, $firstJob); + } else { + // Skip invalid entry, try next line or handle error + // For now, write anyway for backward compatibility + file_put_contents($active, $firstJob); + }
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/include/Control.php (1)
54-61: Variable$fileis shadowed, reducing clarity.The validated
$filefrom line 48 is overwritten on line 56 with the contents read from$local. While functionally correct (the condition on line 57 handlesfalse), using a distinct variable name like$targetFilewould improve readability.Suggested improvement
if ($cancel==1) { if (file_exists($local)) { - $file = file_get_contents($local); - if ($file !== false) delete_file($file); + $targetFile = file_get_contents($local); + if ($targetFile !== false) delete_file($targetFile); } delete_file($local); die('stop'); }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix/include/Control.php
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
Repo: unraid/webgui 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.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-08-14T23:53:38.384Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2338
File: emhttp/plugins/dynamix/nchan/parity_list:151-151
Timestamp: 2025-08-14T23:53:38.384Z
Learning: In the unraid/webgui codebase, the parity_list nchan script always JSON-encodes the $echo payload when publishing to the 'parity' channel, regardless of whether $echo is an array or string. This is the pre-existing behavior and consumers are designed to handle JSON-encoded payloads appropriately.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-10-22T17:36:50.995Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 0
File: :0-0
Timestamp: 2025-10-22T17:36:50.995Z
Learning: In the Unraid webgui codebase, when adjusting CPU pinning, 3rd party containers without template files do not appear in the list. Therefore, silently skipping non-existent template files without logging is the appropriate behavior in UpdateOne.php.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-06-03T21:27:15.912Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2230
File: emhttp/plugins/dynamix/include/Templates.php:63-74
Timestamp: 2025-06-03T21:27:15.912Z
Learning: In the Unraid WebGUI codebase (emhttp/plugins/dynamix/include/Templates.php), there are known duplicate ID issues in checkbox templates across multiple template instances that the maintainers are aware of but have chosen not to address due to the effort required for legacy code improvements.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-02-27T21:50:34.913Z
Learnt from: dlandon
Repo: unraid/webgui PR: 2035
File: emhttp/plugins/dynamix/scripts/syslogfilter:45-48
Timestamp: 2025-02-27T21:50:34.913Z
Learning: The syslog filter script in the unraid/webgui repository does not require explicit error handling for the rsyslog service restart. The simple approach without additional error checking is sufficient for this implementation.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-10-04T05:22:33.141Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2421
File: emhttp/plugins/dynamix/include/DefaultPageLayout/MainContentTabless.php:23-0
Timestamp: 2025-10-04T05:22:33.141Z
Learning: In the Unraid webgui repository, `emhttp/webGui` is a symlink that points to `plugins/dynamix`. Therefore, paths using `$docroot/webGui/...` correctly resolve to `$docroot/plugins/dynamix/...` at runtime.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-03-27T22:04:34.550Z
Learnt from: zackspear
Repo: unraid/webgui 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.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 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/include/Control.php
🧬 Code graph analysis (1)
emhttp/plugins/dynamix/include/Control.php (1)
emhttp/plugins/dynamix/include/PopularDestinations.php (1)
updatePopularDestinations(52-116)
🔇 Additional comments (5)
emhttp/plugins/dynamix/include/Control.php (5)
46-51: Verify intentional GET parameter support for upload operations.Accepting
mode,file,start, andcancelfrom GET parameters exposes these operations to URL-based triggering. While CSRF protection is centralized, GET requests are logged in server access logs and can be cached/bookmarked. Ensure this is intentional for the upload use case.
137-146: LGTM on graceful JSON parsing.The jobs display correctly handles empty lines and invalid JSON entries by skipping them, ensuring robustness when processing the queue file.
184-198: LGTM on undo logic.Processing rows in reverse order correctly preserves index validity during removal. The conversion from 1-based job numbers to 0-based array indices is handled properly.
208-231: LGTM on JSON-based state storage and popular destinations integration.The structured array approach with JSON encoding improves maintainability over the previous INI-like format. The popular destinations update is correctly triggered only for copy/move operations (actions 3, 4, 8, 9) when a target is provided.
201-204: LGTM.Reading the active file as raw content is appropriate since consumers will parse the JSON response.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
emhttp/plugins/dynamix/include/Control.php (1)
140-153: Validate JSON before promoting job to active file.Line 143 writes
$lines[0]directly to the active file without validating it's well-formed JSON. If a legacy INI-format entry or corrupted JSON is first in the queue, the backend will receive invalid data and the job will fail. The 'jobs' case gracefully skips non-JSON lines (line 116), but 'start' does not apply the same safeguard.🔎 Proposed fix
// read first JSON line from jobs file and write to active $lines = file($jobs, FILE_IGNORE_NEW_LINES); if (!empty($lines)) { - file_put_contents($active, $lines[0]); + // Validate JSON before writing to active file + $data = json_decode($lines[0], true); + if (!$data) { + // Skip invalid JSON and try next line + array_shift($lines); + if (count($lines) > 0) { + file_put_contents($jobs, implode("\n", $lines)."\n"); + } else { + delete_file($jobs); + } + die('0'); + } + file_put_contents($active, $lines[0]); // remove first line from jobs fileemhttp/plugins/dynamix/Browse.page (1)
354-534: Remove debugging console.log statements before production.This function contains numerous console.log statements (lines 367, 372, 376-377, 379, 382, 387, 390, 465, 470, 484, 517, 519, 557, 562, 571, 577, 582, 585, 593, 605-609, 615) that should be removed for production code. These create unnecessary console noise and can impact performance.
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/Browse.page (1)
354-534: Consider refactoring for improved maintainability.This 180-line function handles multiple concerns: event prevention, popular destination clicks, tree navigation, input handling, and cleanup. Breaking it into smaller, focused functions would improve readability and testability.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/dynamix/Browse.pageemhttp/plugins/dynamix/include/Control.php
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-12-30T20:27:59.455Z
Learnt from: mgutt
Repo: unraid/webgui PR: 2501
File: emhttp/plugins/dynamix/Browse.page:1282-1339
Timestamp: 2025-12-30T20:27:59.455Z
Learning: In emhttp/plugins/dynamix/Browse.page, the xlink() function's clipboard functionality is intentionally designed to silently fail on HTTP connections without showing warnings. The path text is pre-selected to allow manual copying, which is preferred over showing repetitive error messages that would annoy HTTP users.
Applied to files:
emhttp/plugins/dynamix/Browse.page
📚 Learning: 2025-09-05T19:26:36.587Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2354
File: emhttp/plugins/dynamix/ShareEdit.page:0-0
Timestamp: 2025-09-05T19:26:36.587Z
Learning: In emhttp/plugins/dynamix/ShareEdit.page, the clone-settings div was moved outside the form element and both are wrapped in a div.relative container to prevent event bubbling issues while maintaining proper positioning.
Applied to files:
emhttp/plugins/dynamix/Browse.page
📚 Learning: 2025-08-14T23:53:38.384Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2338
File: emhttp/plugins/dynamix/nchan/parity_list:151-151
Timestamp: 2025-08-14T23:53:38.384Z
Learning: In the unraid/webgui codebase, the parity_list nchan script always JSON-encodes the $echo payload when publishing to the 'parity' channel, regardless of whether $echo is an array or string. This is the pre-existing behavior and consumers are designed to handle JSON-encoded payloads appropriately.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-10-22T17:36:50.995Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 0
File: :0-0
Timestamp: 2025-10-22T17:36:50.995Z
Learning: In the Unraid webgui codebase, when adjusting CPU pinning, 3rd party containers without template files do not appear in the list. Therefore, silently skipping non-existent template files without logging is the appropriate behavior in UpdateOne.php.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-06-03T21:27:15.912Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2230
File: emhttp/plugins/dynamix/include/Templates.php:63-74
Timestamp: 2025-06-03T21:27:15.912Z
Learning: In the Unraid WebGUI codebase (emhttp/plugins/dynamix/include/Templates.php), there are known duplicate ID issues in checkbox templates across multiple template instances that the maintainers are aware of but have chosen not to address due to the effort required for legacy code improvements.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-02-27T21:50:34.913Z
Learnt from: dlandon
Repo: unraid/webgui PR: 2035
File: emhttp/plugins/dynamix/scripts/syslogfilter:45-48
Timestamp: 2025-02-27T21:50:34.913Z
Learning: The syslog filter script in the unraid/webgui repository does not require explicit error handling for the rsyslog service restart. The simple approach without additional error checking is sufficient for this implementation.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-10-04T05:22:33.141Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2421
File: emhttp/plugins/dynamix/include/DefaultPageLayout/MainContentTabless.php:23-0
Timestamp: 2025-10-04T05:22:33.141Z
Learning: In the Unraid webgui repository, `emhttp/webGui` is a symlink that points to `plugins/dynamix`. Therefore, paths using `$docroot/webGui/...` correctly resolve to `$docroot/plugins/dynamix/...` at runtime.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-03-27T22:04:34.550Z
Learnt from: zackspear
Repo: unraid/webgui 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.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 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/include/Control.php
🧬 Code graph analysis (1)
emhttp/plugins/dynamix/include/Control.php (1)
emhttp/plugins/dynamix/include/PopularDestinations.php (1)
updatePopularDestinations(52-116)
🔇 Additional comments (11)
emhttp/plugins/dynamix/include/Control.php (3)
16-16: LGTM: Popular destinations dependency added.The include is correctly positioned in the bootstrap sequence.
113-122: LGTM: Graceful JSON parsing handles legacy entries.The
if (!$data) continue;on line 116 ensures non-JSON or malformed entries are silently skipped, preventing queue display breakage from legacy INI-format jobs.
184-206: Action codes verified as copy/move operations.The condition on line 204 correctly restricts
updatePopularDestinationsto copy and move operations. Action codes 3, 4, 8, and 9 are explicitly documented in Browse.page (line 900) as:
- 3 = copy folder
- 4 = move folder
- 8 = copy file
- 9 = move file
The integration is correct.
emhttp/plugins/dynamix/Browse.page (8)
60-66: LGTM!The function correctly uses namespaced events and event propagation control to prevent the fileTree dropdown from closing when clicking inside dialogs.
536-554: LGTM!The function correctly handles folder collapsing with appropriate early returns and DOM manipulation.
621-644: LGTM!The recursive folder expansion logic correctly handles asynchronous tree expansion with appropriate delays (300ms per level, matching the timing in
navigateFileTree).
698-698: Height increase accommodates new navigation UI.The dialog height increase from 630 to 800 for copy/move operations appropriately accommodates the new file tree navigation features. The change is consistently applied across both single-item (doAction) and batch (doActions) operations.
Also applies to: 708-708, 727-727, 735-735, 998-998, 1010-1010
785-787: LGTM!The cleanup handlers properly remove namespaced event listeners when dialogs close, preventing memory leaks.
Also applies to: 1062-1064
898-903: LGTM!The integration calls correctly wire up the navigation features after dialog creation, with appropriate conditional checks for copy/move operations that require target selection.
Also applies to: 1178-1183
323-323: LGTM!The fix to job queue numbering correctly extracts the queue index, addressing the off-by-one issue mentioned in the PR objectives.
1282-1339: LGTM!The xlink() rewrite implements the new features well:
- Textarea approach provides better mobile support for path selection
- Clipboard functionality gracefully degrades on HTTP (silent fail with pre-selected text for manual copy)
- Terminal integration adds the requested "Open Terminal here" feature
- Success feedback tooltip enhances UX
The
openTerminal()function is properly defined inemhttp/plugins/dynamix/include/DefaultPageLayout/HeadInlineJS.phpand accepts the expected three-parameter signature (tag, name, more), matching the call in xlink().
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
emhttp/plugins/dynamix/nchan/file_manager (1)
355-365: JSON parsing with error logging addresses the previous review feedback.The implementation now logs a warning when JSON parsing fails for non-empty content, which aids debugging without breaking functionality.
emhttp/plugins/dynamix/include/PopularDestinations.php (1)
43-49: File locking and error handling properly implemented.The
LOCK_EXflag prevents concurrent write corruption, and write failures are now logged. This addresses the previous review feedback.emhttp/plugins/dynamix/include/FileTree.php (1)
62-62: The$autocompletevariable is now properly defined.This addresses the previous review comment about the undefined variable.
🧹 Nitpick comments (3)
emhttp/plugins/dynamix/include/FileTree.php (1)
94-94: Consider using translation wrapper for "Popular" label.The "Popular" header text is hardcoded. For consistency with other localized strings in the codebase, consider wrapping it with a translation function.
🔎 Proposed fix
- echo "<li class='popular-header small-caps-label' style='list-style:none;padding:5px 0 5px 20px;'>Popular</li>"; + echo "<li class='popular-header small-caps-label' style='list-style:none;padding:5px 0 5px 20px;'>" . _('Popular') . "</li>";Note: This requires including the Translations.php file in FileTree.php if not already available.
emhttp/plugins/dynamix/Browse.page (2)
585-588: Timing calculation assumes 300ms per folder expansion.The timeout
parts.length * 300 + 500should align with the 300ms delay used inopenFolderRecursive(line 612). Consider extracting this to a named constant for maintainability.🔎 Suggested improvement
// At the top of the script section var FOLDER_EXPAND_DELAY = 300; // In openFolderRecursive (line 610-612) setTimeout(function() { openFolderRecursive($tree, pickroot, parts, index + 1); }, FOLDER_EXPAND_DELAY); // In navigateFileTree (line 585-588) setTimeout(function() { $target.data('isProgrammaticNavigation', false); $target.data('savedInputValue', ''); }, parts.length * FOLDER_EXPAND_DELAY + 500);
323-323: Minor: Variable shadowing withrowarray.The
rowvariable is used both for the array being built and later in the loop iteration. While this works, using a more descriptive name likeselectedRowswould improve readability.🔎 Suggested improvement
- let row = []; - dfm.window.find('i[id^="queue_"]').each(function(){if ($(this).hasClass('fa-check-square-o')) row.push($(this).prop('id').split('_')[1]);}); - $.post('/webGui/include/Control.php',{mode:'undo',row:row.join(',')},function(queue){ + let selectedRows = []; + dfm.window.find('i[id^="queue_"]').each(function(){if ($(this).hasClass('fa-check-square-o')) selectedRows.push($(this).prop('id').split('_')[1]);}); + $.post('/webGui/include/Control.php',{mode:'undo',row:selectedRows.join(',')},function(queue){
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
emhttp/plugins/dynamix/Browse.pageemhttp/plugins/dynamix/include/Control.phpemhttp/plugins/dynamix/include/FileTree.phpemhttp/plugins/dynamix/include/OpenTerminal.phpemhttp/plugins/dynamix/include/PopularDestinations.phpemhttp/plugins/dynamix/nchan/file_manager
🧰 Additional context used
🧠 Learnings (13)
📚 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/include/OpenTerminal.phpemhttp/plugins/dynamix/include/PopularDestinations.phpemhttp/plugins/dynamix/include/Control.phpemhttp/plugins/dynamix/include/FileTree.php
📚 Learning: 2025-09-03T08:33:04.940Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2352
File: emhttp/plugins/dynamix.plugin.manager/include/PluginHelpers.php:0-0
Timestamp: 2025-09-03T08:33:04.940Z
Learning: In the unraid/webgui codebase, Squidly271 prefers file locking to be handled centrally within the file_put_contents_atomic function rather than in individual functions that use it. This maintains cleaner architecture by centralizing the locking mechanism.
Applied to files:
emhttp/plugins/dynamix/include/PopularDestinations.php
📚 Learning: 2025-08-14T23:53:38.384Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2338
File: emhttp/plugins/dynamix/nchan/parity_list:151-151
Timestamp: 2025-08-14T23:53:38.384Z
Learning: In the unraid/webgui codebase, the parity_list nchan script always JSON-encodes the $echo payload when publishing to the 'parity' channel, regardless of whether $echo is an array or string. This is the pre-existing behavior and consumers are designed to handle JSON-encoded payloads appropriately.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-10-22T17:36:50.995Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 0
File: :0-0
Timestamp: 2025-10-22T17:36:50.995Z
Learning: In the Unraid webgui codebase, when adjusting CPU pinning, 3rd party containers without template files do not appear in the list. Therefore, silently skipping non-existent template files without logging is the appropriate behavior in UpdateOne.php.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-06-03T21:27:15.912Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2230
File: emhttp/plugins/dynamix/include/Templates.php:63-74
Timestamp: 2025-06-03T21:27:15.912Z
Learning: In the Unraid WebGUI codebase (emhttp/plugins/dynamix/include/Templates.php), there are known duplicate ID issues in checkbox templates across multiple template instances that the maintainers are aware of but have chosen not to address due to the effort required for legacy code improvements.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-02-27T21:50:34.913Z
Learnt from: dlandon
Repo: unraid/webgui PR: 2035
File: emhttp/plugins/dynamix/scripts/syslogfilter:45-48
Timestamp: 2025-02-27T21:50:34.913Z
Learning: The syslog filter script in the unraid/webgui repository does not require explicit error handling for the rsyslog service restart. The simple approach without additional error checking is sufficient for this implementation.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-10-04T05:22:33.141Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2421
File: emhttp/plugins/dynamix/include/DefaultPageLayout/MainContentTabless.php:23-0
Timestamp: 2025-10-04T05:22:33.141Z
Learning: In the Unraid webgui repository, `emhttp/webGui` is a symlink that points to `plugins/dynamix`. Therefore, paths using `$docroot/webGui/...` correctly resolve to `$docroot/plugins/dynamix/...` at runtime.
Applied to files:
emhttp/plugins/dynamix/include/Control.phpemhttp/plugins/dynamix/include/FileTree.php
📚 Learning: 2025-03-27T22:04:34.550Z
Learnt from: zackspear
Repo: unraid/webgui 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.
Applied to files:
emhttp/plugins/dynamix/include/Control.phpemhttp/plugins/dynamix/include/FileTree.php
📚 Learning: 2025-02-27T21:52:42.541Z
Learnt from: dlandon
Repo: unraid/webgui PR: 2035
File: emhttp/plugins/dynamix/SyslogFilter.page:19-26
Timestamp: 2025-02-27T21:52:42.541Z
Learning: In the unraid/webgui repository, basic error handling is sufficient for file operations like checking file existence and using the @ operator to suppress errors, without additional error messages or notifications.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
Repo: unraid/webgui 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.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manageremhttp/plugins/dynamix/include/FileTree.php
📚 Learning: 2025-02-27T21:53:37.589Z
Learnt from: dlandon
Repo: unraid/webgui PR: 2035
File: etc/rc.d/rc.rsyslogd:48-50
Timestamp: 2025-02-27T21:53:37.589Z
Learning: For the unraid/webgui project, simple error handling is preferred over extensive error checking in shell scripts. The copy operation for configuration files like in rc.rsyslogd doesn't require additional error logging, permission checks, or return statements.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-12-30T20:27:59.455Z
Learnt from: mgutt
Repo: unraid/webgui PR: 2501
File: emhttp/plugins/dynamix/Browse.page:1282-1339
Timestamp: 2025-12-30T20:27:59.455Z
Learning: In emhttp/plugins/dynamix/Browse.page, the xlink() function's clipboard functionality is intentionally designed to silently fail on HTTP connections without showing warnings. The path text is pre-selected to allow manual copying, which is preferred over showing repetitive error messages that would annoy HTTP users.
Applied to files:
emhttp/plugins/dynamix/Browse.page
📚 Learning: 2025-09-05T19:26:36.587Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2354
File: emhttp/plugins/dynamix/ShareEdit.page:0-0
Timestamp: 2025-09-05T19:26:36.587Z
Learning: In emhttp/plugins/dynamix/ShareEdit.page, the clone-settings div was moved outside the form element and both are wrapped in a div.relative container to prevent event bubbling issues while maintaining proper positioning.
Applied to files:
emhttp/plugins/dynamix/Browse.page
🧬 Code graph analysis (2)
emhttp/plugins/dynamix/include/Control.php (1)
emhttp/plugins/dynamix/include/PopularDestinations.php (1)
updatePopularDestinations(55-119)
emhttp/plugins/dynamix/include/FileTree.php (1)
emhttp/plugins/dynamix/include/PopularDestinations.php (1)
getPopularDestinations(126-141)
🔇 Additional comments (17)
emhttp/plugins/dynamix/nchan/file_manager (3)
132-156: Docblock and reset logic look good.The docblock properly documents the function's purpose, parameters, and the static-state reset behavior. The reset mechanism correctly clears all static variables when
$resetistrue.
178-210: Multi-sample averaging for total size estimation is well implemented.The approach of collecting up to 5 measurements at different progress percentages and averaging them compensates for rsync's truncation behavior. The logic correctly:
- Only samples from "running transfer" lines (no
xfr#info)- Waits for at least 3% progress before sampling
- Tracks
$last_calc_percentto avoid duplicate samples at the same percentage- Adds 0.5% adjustment to compensate for truncation
427-427: Reset calls before copy/move operations are correctly placed.Calling
parse_rsync_progress(null, null, true)before starting copy (line 427) and move (line 471) operations ensures clean state and prevents cross-operation contamination.Also applies to: 471-471
emhttp/plugins/dynamix/include/PopularDestinations.php (2)
98-114: Re-indexing after array_filter is correctly handled.Line 114 (
$destinations = array_values($destinations)) properly re-indexes the array afterarray_filterremoves entries with score ≤ 0, ensuring consistent 0-based numeric keys in the saved JSON.
55-96: Add defensive normalization on load for robustness.While the current code prevents trailing-slash paths under normal operation (by normalizing before storage), the
loadPopularDestinations()function should normalize loaded paths as well. This ensures consistency if the JSON file is modified externally or populated from legacy data sources, preventing duplicate entries from paths like/mnt/disk1and/mnt/disk1/.emhttp/plugins/dynamix/include/OpenTerminal.php (3)
76-84: Startup script generation looks correct.The heredoc properly sources the modified profile, handles
.bash_profileloading, cleans up temporary files, and launches an interactive bash session. The use ofexec bash --norc -iprevents double-sourcing of rc files.
62-66: Path validation with realpath fallback is appropriate.Using
realpath()validates the path exists and resolves symlinks. Falling back to/rootfor invalid paths is a safe default.
70-72: The escaping implementation at lines 70-72 is correct and does not suffer from the re-escaping concern raised. PHP'sstr_replace()with an array argument processes all replacements simultaneously rather than sequentially, preventing cascading escapes. Testing confirms that when both&and\are present in a path (e.g.,/mnt/test&\mixed), they are each escaped exactly once (\&and\\respectively) without double-escaping. The code properly escapes sed metacharacters for safe use in the replacement string.Likely an incorrect or invalid review comment.
emhttp/plugins/dynamix/include/Control.php (2)
113-122: JSON parsing for jobs listing handles edge cases correctly.The code properly skips empty rows and invalid JSON entries with
continue, preventing display issues from malformed data.
216-220: Popular destinations update integration is correct.Updating popular destinations for copy/move actions (3, 4, 8, 9) with non-empty targets is the appropriate behavior for tracking frequently used destinations.
emhttp/plugins/dynamix/include/FileTree.php (2)
76-91: FUSE conflict filtering logic is comprehensive.The filtering correctly handles:
/mnt/userand/mnt/rootsharecontexts: only shows paths from the same context or external mounts/mnt/diskXor/mnt/cachecontexts: excludes user share paths- Non-
/mnt/roots: no filtering neededThis prevents confusing users by showing inaccessible paths.
93-107: Popular destinations UI rendering is well-structured.Using
data-pathinstead ofrelattribute correctly prevents jQueryFileTree from intercepting clicks, allowing custom handling in JavaScript. The separator line provides clear visual distinction.emhttp/plugins/dynamix/Browse.page (5)
60-66: Event propagation prevention for fileTree is correctly implemented.Using
stopPropagation()on mousedown events within the dialog prevents the document-level handler in jQueryFileTree from closing the dropdown when interacting with the dialog.
376-378: Retain console.error for actual error conditions.The
console.erroron line 377 is appropriate for logging actual errors (tree show failures), unlike debugconsole.logstatements which should be removed. This is acceptable for production.
508-517: Dialog close cleanup properly removes event listeners.The cleanup function correctly removes all three capture-phase event listeners (treeClickHandler, popularClickHandler, preventClose handlers), preventing memory leaks and stale handlers.
755-757: Dialog close handlers properly detach fileTree event handlers.Both
doActionanddoActionsdialogs now clean up the.dfmFileTreenamespace handlers on close, ensuring proper cleanup.Also applies to: 1032-1034
1252-1309: Clipboard functionality correctly handles HTTP limitations.Based on the learnings, the silent failure on HTTP is intentional. The path is pre-selected for manual copying, and the Terminal button provides an alternative workflow. This is a pragmatic UX decision.
- Extract rsync progress thresholds as constants (RSYNC_MIN_PROGRESS_PERCENT, RSYNC_TOTAL_SIZE_SAMPLES) - Add helper functions for unit conversion (size_to_bytes, bytes_to_size) - Use explicit int casting for action type comparisons in Control.php - Reduces code duplication and improves maintainability Addresses PR feedback for code quality improvements.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/nchan/file_manager (1)
84-120: Well-implemented size conversion helpers.The
size_to_bytesandbytes_to_sizefunctions are well-documented and correctly handle unit conversions. The case-insensitive unit detection withstriposis appropriate.Optional: Consider locale-independent formatting
The
number_format()function inbytes_to_sizeis locale-dependent for decimal separators (e.g., "1,50GB" in some locales vs "1.50GB"). If this output is ever parsed programmatically, consider usingsprintf('%.2f', $display)instead. For display-only purposes, the current implementation is fine.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
emhttp/plugins/dynamix/Browse.pageemhttp/plugins/dynamix/include/Control.phpemhttp/plugins/dynamix/nchan/file_manager
🧰 Additional context used
🧠 Learnings (14)
📚 Learning: 2025-08-14T23:53:38.384Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2338
File: emhttp/plugins/dynamix/nchan/parity_list:151-151
Timestamp: 2025-08-14T23:53:38.384Z
Learning: In the unraid/webgui codebase, the parity_list nchan script always JSON-encodes the $echo payload when publishing to the 'parity' channel, regardless of whether $echo is an array or string. This is the pre-existing behavior and consumers are designed to handle JSON-encoded payloads appropriately.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-10-22T17:36:50.995Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 0
File: :0-0
Timestamp: 2025-10-22T17:36:50.995Z
Learning: In the Unraid webgui codebase, when adjusting CPU pinning, 3rd party containers without template files do not appear in the list. Therefore, silently skipping non-existent template files without logging is the appropriate behavior in UpdateOne.php.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-06-03T21:27:15.912Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2230
File: emhttp/plugins/dynamix/include/Templates.php:63-74
Timestamp: 2025-06-03T21:27:15.912Z
Learning: In the Unraid WebGUI codebase (emhttp/plugins/dynamix/include/Templates.php), there are known duplicate ID issues in checkbox templates across multiple template instances that the maintainers are aware of but have chosen not to address due to the effort required for legacy code improvements.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-02-27T21:50:34.913Z
Learnt from: dlandon
Repo: unraid/webgui PR: 2035
File: emhttp/plugins/dynamix/scripts/syslogfilter:45-48
Timestamp: 2025-02-27T21:50:34.913Z
Learning: The syslog filter script in the unraid/webgui repository does not require explicit error handling for the rsyslog service restart. The simple approach without additional error checking is sufficient for this implementation.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-08-13T03:13:53.283Z
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.
Applied to files:
emhttp/plugins/dynamix/include/Control.phpemhttp/plugins/dynamix/Browse.page
📚 Learning: 2025-10-03T02:57:29.994Z
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.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
Repo: unraid/webgui 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.
Applied to files:
emhttp/plugins/dynamix/include/Control.phpemhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-10-04T05:22:33.141Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2421
File: emhttp/plugins/dynamix/include/DefaultPageLayout/MainContentTabless.php:23-0
Timestamp: 2025-10-04T05:22:33.141Z
Learning: In the Unraid webgui repository, `emhttp/webGui` is a symlink that points to `plugins/dynamix`. Therefore, paths using `$docroot/webGui/...` correctly resolve to `$docroot/plugins/dynamix/...` at runtime.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-03-27T22:04:34.550Z
Learnt from: zackspear
Repo: unraid/webgui 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.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 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/include/Control.php
📚 Learning: 2025-12-30T20:27:59.455Z
Learnt from: mgutt
Repo: unraid/webgui PR: 2501
File: emhttp/plugins/dynamix/Browse.page:1282-1339
Timestamp: 2025-12-30T20:27:59.455Z
Learning: In emhttp/plugins/dynamix/Browse.page, the xlink() function's clipboard functionality is intentionally designed to silently fail on HTTP connections without showing warnings. The path text is pre-selected to allow manual copying, which is preferred over showing repetitive error messages that would annoy HTTP users.
Applied to files:
emhttp/plugins/dynamix/Browse.page
📚 Learning: 2025-09-05T19:26:36.587Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2354
File: emhttp/plugins/dynamix/ShareEdit.page:0-0
Timestamp: 2025-09-05T19:26:36.587Z
Learning: In emhttp/plugins/dynamix/ShareEdit.page, the clone-settings div was moved outside the form element and both are wrapped in a div.relative container to prevent event bubbling issues while maintaining proper positioning.
Applied to files:
emhttp/plugins/dynamix/Browse.page
📚 Learning: 2025-05-31T00:04:34.204Z
Learnt from: zackspear
Repo: unraid/webgui PR: 0
File: :0-0
Timestamp: 2025-05-31T00:04:34.204Z
Learning: For the unraid/webgui legacy codebase, ARIA labels and accessibility concerns should not be addressed in individual PRs. These will be handled via a different solution in the future.
Applied to files:
emhttp/plugins/dynamix/Browse.page
📚 Learning: 2025-02-27T21:52:42.541Z
Learnt from: dlandon
Repo: unraid/webgui PR: 2035
File: emhttp/plugins/dynamix/SyslogFilter.page:19-26
Timestamp: 2025-02-27T21:52:42.541Z
Learning: In the unraid/webgui repository, basic error handling is sufficient for file operations like checking file existence and using the @ operator to suppress errors, without additional error messages or notifications.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
🧬 Code graph analysis (1)
emhttp/plugins/dynamix/include/Control.php (1)
emhttp/plugins/dynamix/include/PopularDestinations.php (1)
updatePopularDestinations(55-119)
🔇 Additional comments (22)
emhttp/plugins/dynamix/include/Control.php (4)
16-16: LGTM!The PopularDestinations module inclusion is correctly placed and properly utilized later in the file (line 223).
140-172: LGTM! Past review concern addressed.The validation loop (lines 144-152) correctly skips invalid JSON entries until a valid one is found, and properly handles the case when no valid entries remain (lines 154-158). This successfully addresses the past review comment about the problematic
returnstatement that would have exited the script entirely.The implementation now correctly processes remaining entries without exiting the included script.
201-224: LGTM! Well-structured integration.The structured data array (lines 201-210) with explicit type casting is cleaner than string assembly. The popular destinations integration (lines 220-224) is correctly implemented:
- Only tracks copy/move operations (actions 3, 4, 8, 9)
- Validates target is not empty before updating
- Executes for both queued and immediate operations
- Aligns with the PopularDestinations API (which filters dangerous paths like
/mntand/boot)
113-122: Confirm the carriage return delimiter on line 118 is intentional.The code correctly handles JSON parsing and invalid entries with appropriate null checks and continue statements. However, line 118 uses
"\r"(carriage return) as the delimiter for splitting multiple source paths, which is an unusual choice. Standard practice would be"\n"(line feed) or"\r\n"(CRLF).This appears to be working code, but please verify:
- The
sourcefield from the client is actually separated by carriage returns- If this delimiter choice was deliberate or inherited from legacy code
If
"\r"is incorrect, it should be changed to match how the client constructs the source field.emhttp/plugins/dynamix/nchan/file_manager (8)
24-27: LGTM!The named constants for rsync progress calculation improve code readability and maintainability. The threshold values (3% minimum progress, 5 samples for averaging) are reasonable for balancing accuracy with responsiveness.
122-158: LGTM! ETA calculation with hysteresis is well-implemented.The refactored
calculate_etafunction correctly uses the new byte conversion helpers and implements reasonable hysteresis (70/30 blend) to smooth out ETA fluctuations. Edge cases for 0% progress and low/zero speed are properly handled.
160-184: Excellent documentation and reset mechanism.The comprehensive PHPDoc clearly explains the static state management and reset behavior. The
$resetparameter implementation properly clears all static variables to prevent state leakage between operations. This addresses the previous review comment requesting documentation.
206-231: LGTM! Total size estimation with multi-sample averaging is sound.The total size calculation logic is well-designed. The 0.5% compensation for rsync's percent truncation and the multi-sample averaging strategy at different progress points should achieve the claimed ~2% accuracy. The conditional logic correctly prevents recalculation at the same percent and stops array growth after reaching the sample limit.
243-256: LGTM! Progress text properly internationalized.The progress text construction correctly wraps all user-facing strings with
_()for translation. The "~" prefix on the total size appropriately indicates that it's an approximation, and the fallback to "N/A" gracefully handles cases where the total hasn't been calculated yet.
365-375: LGTM! JSON parameter loading with proper error handling.The switch from INI to JSON-based parameter loading is well-implemented. The error logging for parse failures (previously requested) has been properly addressed, using
loggerwith safely escaped content limited to 100 characters to prevent log spam.
437-437: LGTM! Static state reset properly called at operation boundaries.The
parse_rsync_progress(null, null, true)calls correctly reset static variables before starting new copy and move operations, preventing state leakage between transfers as documented in the function's PHPDoc.Also applies to: 481-481
447-447: LGTM! Error messages properly internationalized.The error messages are correctly wrapped with
_()for translation support, addressing the requirement from issue #2500 to enable localization of hardcoded English messages.Also applies to: 548-548, 578-578
emhttp/plugins/dynamix/Browse.page (10)
60-66: LGTM! Clean event handler management.The function correctly prevents fileTree dropdown closure by stopping mousedown propagation. The use of namespaced events (
.dfmFileTree) enables targeted cleanup later.
354-356: Well-named constants improve maintainability.Extracting timing values into clearly named constants (FOLDER_EXPAND_DELAY, NAVIGATION_BUFFER) makes the navigation timing logic self-documenting and easier to tune.
358-522: Complex orchestration handled well.This 160+ line function coordinates multiple concerns (event prevention, popular destinations, navigation, input handling), but the implementation is sound:
- Capture-phase event handling correctly prevents tree closure
- Flag management (isNavigatingFromPopular, isProgrammaticNavigation) prevents recursive triggers
- Comprehensive cleanup on dialog close prevents memory leaks
- Debouncing (200ms timeout) prevents excessive navigation calls
The mixing of native
addEventListener(required for capture phase) with jQuery handlers is necessary and properly managed.
524-618: Well-designed navigation helpers.The four helper functions form a cohesive API:
closeFolderPath()cleanly collapses folders by toggling classes and removing child elementsresetFileTree()properly sequences hide → empty → reload with setTimeoutnavigateFileTree()usesjQuery.data()to pass state without polluting global scopeopenFolderRecursive()handles all cases (already expanded, collapsed, base case) with proper delaysAll functions check element existence before operating and use the named timing constants consistently.
323-323: Job queue ID extraction is correct.The change properly extracts the queue number from element IDs like
queue_1,queue_2, etc. by splitting on underscore and taking the second element.
672-672: Dialog height increase accommodates enhanced navigation UI.Increasing height from 630 to 800 pixels for copy/move dialogs is appropriate given the addition of the FileTree browser with popular destinations and manual navigation features.
Also applies to: 682-682, 701-701, 709-709, 972-972, 984-984
759-761: Proper event handler cleanup prevents memory leaks.The dialog close handlers correctly remove the namespaced
mousedown.dfmFileTreeevent handlers, complementing the more comprehensive cleanup insetupTargetNavigation(). This prevents memory leaks when dialogs are destroyed.Also applies to: 1036-1038
872-877: Navigation setup correctly scoped to copy/move operations.The calls to
preventFileTreeClose()andsetupTargetNavigation()are properly placed:
preventFileTreeClose()applies to all dialogs to prevent tree closuresetupTargetNavigation()conditionally applies only to copy/move operations (actions 3, 4, 8, 9 indoAction; 3, 4 indoActions)- The additional check for
#dfm_targetexistence ensures the field is present before attaching handlersAlso applies to: 1152-1157
1256-1313: Clipboard integration with graceful HTTP degradation.The rewritten
xlink()function handles clipboard operations well:
- Pre-selects path text to enable manual copying (works on all protocols)
- Attempts automatic clipboard copy via
navigator.clipboard.writeText()on HTTPS- Silent failure on HTTP is intentional (per established learnings) to avoid repetitive warnings
- Shows success tooltip only when clipboard API succeeds
- Terminal button provides additional functionality
Based on learnings, this approach balances functionality with user experience.
378-382: Appropriate error logging in exception handler.The
console.error()statement in the catch block is appropriate production error logging (not debugging output). It captures unexpected failures when showing the tree while preventing the error from propagating to the user.
… class - Changed all warning spans from dfm_text to dfm_warning class - Prevents BrowseButton.page status updates from overwriting warnings - Moved warnings in Copy/Move templates to appear after target input (footer position) - Affected templates: CopyFolder, MoveFolder, CopyFile, MoveFile, CopyObject, MoveObject - All 15 templates with warnings now properly separated from status updates
- Clone dfm_warning from template and add to ui-dialog-buttonpane - Floats left next to Start/Queue/Cancel buttons - Only enabled for action 3 (copy folder) for testing
- Small dialogs (Create/Delete/Rename) now have less empty space - Copy/Move dialogs still expand automatically due to FileTree content - Removes need for per-action height management
- Copy/Move dialogs (actions 3,4,8,9) now have sufficient space for FileTree - Prevents excessive scrolling in dialogs with floating FileTree dropdown - Applies to both single and bulk operations
- fileTreeAttach() doesn't return a chainable jQuery object in bulk operations - Split CSS application into separate statement to avoid TypeError
…olled All JavaScript dfm.height assignments have been removed as they are overridden managed through default-dynamix.css for consistent behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
emhttp/plugins/dynamix/Browse.page (1)
669-704: Use a fixed pixel value or responsive approach for margin-bottom to accommodate the fileTree dropdown consistently.The
margin-bottom: 20vhprovides inconsistent spacing across screen sizes. On typical viewports (640-1080px height), 20vh equals only 128-216px, which is insufficient for the 300px max-height fileTree dropdown. On very large screens, it becomes excessive. Consider using a fixed value like320px(accounting for the 300px dropdown plus padding) or calculating it dynamically based on remaining viewport space.
🧹 Nitpick comments (2)
emhttp/plugins/dynamix/styles/default-dynamix.css (1)
1501-1506: Use CSS variable for consistent theming.The hardcoded color
#888should use a CSS variable for consistency with the rest of the stylesheet. Consider replacing withvar(--gray-500)or an appropriate variable that matches the intended gray shade.🔎 Proposed fix
.dfm-warning { margin-right: 10px; display: inline-block; vertical-align: middle; - color: #888; + color: var(--gray-500); }emhttp/plugins/dynamix/Browse.page (1)
354-522: Consider future refactoring to reduce complexity.This 169-line function handles multiple concerns (event prevention, popular destination clicks, tree navigation, input handling, and cleanup). While the implementation is functional and the logic is sound, consider extracting smaller helper functions in a future refactor to improve maintainability and testability.
Current implementation is acceptable for this PR, as the function:
- Properly uses capture-phase event listeners
- Correctly manages state flags
- Includes appropriate cleanup handlers
- Uses well-defined timing constants
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
emhttp/plugins/dynamix/Browse.pageemhttp/plugins/dynamix/include/Templates.phpemhttp/plugins/dynamix/styles/default-dynamix.css
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-12-30T20:27:59.455Z
Learnt from: mgutt
Repo: unraid/webgui PR: 2501
File: emhttp/plugins/dynamix/Browse.page:1282-1339
Timestamp: 2025-12-30T20:27:59.455Z
Learning: In emhttp/plugins/dynamix/Browse.page, the xlink() function's clipboard functionality is intentionally designed to silently fail on HTTP connections without showing warnings. The path text is pre-selected to allow manual copying, which is preferred over showing repetitive error messages that would annoy HTTP users.
Applied to files:
emhttp/plugins/dynamix/Browse.page
📚 Learning: 2025-09-05T19:26:36.587Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2354
File: emhttp/plugins/dynamix/ShareEdit.page:0-0
Timestamp: 2025-09-05T19:26:36.587Z
Learning: In emhttp/plugins/dynamix/ShareEdit.page, the clone-settings div was moved outside the form element and both are wrapped in a div.relative container to prevent event bubbling issues while maintaining proper positioning.
Applied to files:
emhttp/plugins/dynamix/Browse.pageemhttp/plugins/dynamix/styles/default-dynamix.cssemhttp/plugins/dynamix/include/Templates.php
📚 Learning: 2025-05-31T00:04:34.204Z
Learnt from: zackspear
Repo: unraid/webgui PR: 0
File: :0-0
Timestamp: 2025-05-31T00:04:34.204Z
Learning: For the unraid/webgui legacy codebase, ARIA labels and accessibility concerns should not be addressed in individual PRs. These will be handled via a different solution in the future.
Applied to files:
emhttp/plugins/dynamix/Browse.page
📚 Learning: 2025-08-13T03:13:53.283Z
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.
Applied to files:
emhttp/plugins/dynamix/Browse.page
📚 Learning: 2025-06-21T00:10:40.789Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2258
File: emhttp/plugins/dynamix/DashStats.page:0-0
Timestamp: 2025-06-21T00:10:40.789Z
Learning: In the Unraid webgui codebase (emhttp/plugins/dynamix), replacing `<i>` elements with `<button>` elements for accessibility would require extensive CSS refactoring due to legacy CSS having direct button styles that would conflict with icon-based toggles.
Applied to files:
emhttp/plugins/dynamix/include/Templates.php
📚 Learning: 2025-06-03T21:27:15.912Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2230
File: emhttp/plugins/dynamix/include/Templates.php:63-74
Timestamp: 2025-06-03T21:27:15.912Z
Learning: In the Unraid WebGUI codebase (emhttp/plugins/dynamix/include/Templates.php), there are known duplicate ID issues in checkbox templates across multiple template instances that the maintainers are aware of but have chosen not to address due to the effort required for legacy code improvements.
Applied to files:
emhttp/plugins/dynamix/include/Templates.php
📚 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/include/Templates.php
🧬 Code graph analysis (1)
emhttp/plugins/dynamix/include/Templates.php (1)
emhttp/plugins/dynamix/include/Helpers.php (1)
mk_option(182-184)
🔇 Additional comments (12)
emhttp/plugins/dynamix/styles/default-dynamix.css (2)
1487-1487: LGTM! Dialog min-height reduction improves smaller dialog layouts.The reduction from 35vh to 20vh allows for more compact dialogs, which aligns with the PR's objective to reduce dialog min-height for smaller dialogs and accommodate the FileTree in Copy/Move dialogs.
1754-1770: LGTM! Responsive design properly handles mobile layouts.The media query appropriately adapts the warning and button layout for mobile devices, ensuring warnings remain readable and buttons are accessible on smaller screens.
emhttp/plugins/dynamix/include/Templates.php (1)
263-285: LGTM! Removal of "narrow" class improves permission control usability.The removal of the
narrowclass from the permission select elements directly addresses the PR objective to "Improve permission-selection control width/usability." The controls will now use the standarddfmclass width, making them easier to use.emhttp/plugins/dynamix/Browse.page (9)
60-66: LGTM! Clean event handler to prevent fileTree closure.The function appropriately uses event propagation control to keep the fileTree open during dialog interactions. The
.off().on()pattern ensures handlers are not duplicated.
524-542: LGTM! Clean implementation of folder collapse logic.The function properly handles folder closure by updating classes and removing child elements. The early return when the folder link is not found prevents errors.
544-562: LGTM! Proper tree reset implementation.The function correctly resets the tree by emptying the container and triggering a reload. The setTimeout delay ensures the DOM update is processed before the click event.
564-618: LGTM! Well-structured recursive navigation implementation.The functions properly handle programmatic tree navigation:
- Clean path parsing and validation
- Appropriate use of state flags via jQuery.data()
- Recursive folder expansion with proper timing using constants
- Handles already-expanded folders efficiently
323-323: LGTM! Job queue numbering correction.The code correctly extracts the queue index from the element ID. This addresses the off-by-one error mentioned in the PR summary.
750-773: LGTM! Warning generation properly integrated into dialogs.The warning text generation in the
openhandler successfully moves the logic from static templates to dynamic JavaScript, as intended by the PR. The warnings are now rendered in the button pane with appropriate icons and i18n support.
884-889: LGTM! Proper integration of navigation helpers.The calls to
preventFileTreeClose()andsetupTargetNavigation()are correctly placed after dialog creation and properly scoped to copy/move actions (3, 4, 8, 9).
1278-1335: LGTM! Well-designed clipboard functionality with Terminal integration.The rewritten
xlink()function provides an excellent user experience:
- Always displays a selectable textarea (mobile-friendly)
- Integrates Terminal access via the Cancel button
- Implements clipboard copy on HTTPS with silent fallback on HTTP
- Pre-selects text for easy manual copying
The silent failure on HTTP is intentional per established learnings, avoiding repetitive warnings while still allowing manual copy.
[approve_code_challenges]Based on learnings, the clipboard approach (silent fail on HTTP) is the preferred UX pattern for this codebase.
1295-1296: TheopenTerminal()function is properly defined and accepts the three parameters being passed from this code. The function signature inemhttp/plugins/dynamix/include/DefaultPageLayout/HeadInlineJS.php(line 158) isfunction openTerminal(tag,name,more), which correctly matches the call:openTerminal('ttyd', 'File_Manager_' + d.getTime(), path). No issues identified.
- Control.php: Remove array_reverse() in undo case (unnecessary with direct indices) - Control.php: Rename shadowed $file variable to $targetFile for clarity - file_manager: Add Helpers.php and use my_number() for locale-aware number formatting - FileTree.php: Add array_values() after array_filter() for proper array reindexing - FileTree.php: Translate 'Popular' label using _() function - PopularDestinations.php: Add path traversal validation (..) blocking with logging
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/include/PopularDestinations.php (1)
70-124: Potential race condition in load-modify-save cycle.The
updatePopularDestinationsfunction performs a read-modify-write cycle without holding a lock during the entire operation. If two copy/move operations complete simultaneously, one update could overwrite the other's changes. WhileLOCK_EXonfile_put_contentsprevents file corruption, it doesn't prevent lost updates.For a non-critical feature like "popular destinations," this is acceptable since data loss is minor (just usage statistics). However, consider using file locking for the entire operation if accuracy becomes important.
🔎 Optional fix using file locking for the entire operation
function updatePopularDestinations($targetPath) { // Skip empty paths or paths that are just /mnt or /boot if (empty($targetPath) || $targetPath == '/mnt' || $targetPath == '/boot') { return; } // Block path traversal attempts if (strpos($targetPath, '..') !== false) { exec('logger -t webGUI "Security: Blocked path traversal attempt in popular destinations: ' . escapeshellarg($targetPath) . '"'); return; } // Normalize path (remove trailing slash) $targetPath = rtrim($targetPath, '/'); + // Acquire exclusive lock for entire read-modify-write cycle + $lockFile = POPULAR_DESTINATIONS_FILE . '.lock'; + $lock = fopen($lockFile, 'c'); + if (!flock($lock, LOCK_EX)) { + fclose($lock); + return; + } + // Load current data $data = loadPopularDestinations(); // ... rest of the function ... // Save $data['destinations'] = $destinations; savePopularDestinations($data); + + flock($lock, LOCK_UN); + fclose($lock); }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
emhttp/plugins/dynamix/include/Control.phpemhttp/plugins/dynamix/include/FileTree.phpemhttp/plugins/dynamix/include/PopularDestinations.phpemhttp/plugins/dynamix/nchan/file_manager
🧰 Additional context used
🧠 Learnings (12)
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
Repo: unraid/webgui 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.
Applied to files:
emhttp/plugins/dynamix/include/FileTree.phpemhttp/plugins/dynamix/nchan/file_manageremhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-10-04T05:22:33.141Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2421
File: emhttp/plugins/dynamix/include/DefaultPageLayout/MainContentTabless.php:23-0
Timestamp: 2025-10-04T05:22:33.141Z
Learning: In the Unraid webgui repository, `emhttp/webGui` is a symlink that points to `plugins/dynamix`. Therefore, paths using `$docroot/webGui/...` correctly resolve to `$docroot/plugins/dynamix/...` at runtime.
Applied to files:
emhttp/plugins/dynamix/include/FileTree.phpemhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-03-27T22:04:34.550Z
Learnt from: zackspear
Repo: unraid/webgui 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.
Applied to files:
emhttp/plugins/dynamix/include/FileTree.phpemhttp/plugins/dynamix/include/Control.php
📚 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/include/FileTree.phpemhttp/plugins/dynamix/include/PopularDestinations.phpemhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-09-03T08:33:04.940Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2352
File: emhttp/plugins/dynamix.plugin.manager/include/PluginHelpers.php:0-0
Timestamp: 2025-09-03T08:33:04.940Z
Learning: In the unraid/webgui codebase, Squidly271 prefers file locking to be handled centrally within the file_put_contents_atomic function rather than in individual functions that use it. This maintains cleaner architecture by centralizing the locking mechanism.
Applied to files:
emhttp/plugins/dynamix/include/PopularDestinations.php
📚 Learning: 2025-02-27T21:52:42.541Z
Learnt from: dlandon
Repo: unraid/webgui PR: 2035
File: emhttp/plugins/dynamix/SyslogFilter.page:19-26
Timestamp: 2025-02-27T21:52:42.541Z
Learning: In the unraid/webgui repository, basic error handling is sufficient for file operations like checking file existence and using the @ operator to suppress errors, without additional error messages or notifications.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-08-14T23:53:38.384Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2338
File: emhttp/plugins/dynamix/nchan/parity_list:151-151
Timestamp: 2025-08-14T23:53:38.384Z
Learning: In the unraid/webgui codebase, the parity_list nchan script always JSON-encodes the $echo payload when publishing to the 'parity' channel, regardless of whether $echo is an array or string. This is the pre-existing behavior and consumers are designed to handle JSON-encoded payloads appropriately.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-10-22T17:36:50.995Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 0
File: :0-0
Timestamp: 2025-10-22T17:36:50.995Z
Learning: In the Unraid webgui codebase, when adjusting CPU pinning, 3rd party containers without template files do not appear in the list. Therefore, silently skipping non-existent template files without logging is the appropriate behavior in UpdateOne.php.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-06-03T21:27:15.912Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2230
File: emhttp/plugins/dynamix/include/Templates.php:63-74
Timestamp: 2025-06-03T21:27:15.912Z
Learning: In the Unraid WebGUI codebase (emhttp/plugins/dynamix/include/Templates.php), there are known duplicate ID issues in checkbox templates across multiple template instances that the maintainers are aware of but have chosen not to address due to the effort required for legacy code improvements.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-02-27T21:50:34.913Z
Learnt from: dlandon
Repo: unraid/webgui PR: 2035
File: emhttp/plugins/dynamix/scripts/syslogfilter:45-48
Timestamp: 2025-02-27T21:50:34.913Z
Learning: The syslog filter script in the unraid/webgui repository does not require explicit error handling for the rsyslog service restart. The simple approach without additional error checking is sufficient for this implementation.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-08-13T03:13:53.283Z
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.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-10-03T02:57:29.994Z
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.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
🧬 Code graph analysis (2)
emhttp/plugins/dynamix/include/FileTree.php (1)
emhttp/plugins/dynamix/include/PopularDestinations.php (1)
getPopularDestinations(132-147)
emhttp/plugins/dynamix/include/Control.php (1)
emhttp/plugins/dynamix/include/PopularDestinations.php (1)
updatePopularDestinations(55-125)
🔇 Additional comments (19)
emhttp/plugins/dynamix/include/PopularDestinations.php (3)
25-38: LGTM on load function with graceful fallbacks.The function handles missing files and malformed JSON gracefully by returning a default structure. This prevents crashes when the file doesn't exist or is corrupted.
55-65: Security checks look appropriate.Path traversal prevention with
..check and logging of blocked attempts is good. Theescapeshellarg()usage in the log message properly escapes the potentially malicious input.
132-147: Redundant sorting ingetPopularDestinations.The comment acknowledges data should already be sorted after
updatePopularDestinations, but sorts anyway "just in case." This is defensive but adds unnecessary overhead for each call. Consider removing if performance becomes a concern, or keep it for robustness against manual file edits.emhttp/plugins/dynamix/include/Control.php (5)
16-16: LGTM on dependency inclusion.The PopularDestinations module is properly included for use in the file case.
61-69: Improved upload handling with explicit target file variable.The refactoring to use
$targetFileimproves code clarity and ensures consistent file path usage throughout the upload handling block.
113-122: JSON parsing with proper validation.The jobs case now correctly parses JSON lines with appropriate empty/invalid checks. The use of
json_decode($row, true)with!$datacheck handles both null (invalid JSON) and empty results.
140-170: Start case properly handles invalid JSON entries with loop.The while loop approach correctly addresses the previous review's concern about the
returnstatement. Invalid JSON entries are skipped until a valid one is found or the queue is exhausted.
219-224: Popular destinations integration looks correct.The integration properly filters for copy/move operations (actions 3, 4, 8, 9) and only updates when a non-empty target is present. This matches the expected behavior for tracking destination usage.
emhttp/plugins/dynamix/include/FileTree.php (3)
53-53: Dependency and variable definition properly added.The PopularDestinations include and
$autocompletevariable extraction from POST data address the previous review comment.Also applies to: 62-62
71-91: FUSE conflict prevention logic is well-designed.The filtering correctly prevents displaying
/mnt/userpaths when browsing/mnt/diskX(and vice versa), which would cause FUSE conflicts. The logic handles three contexts: user context, diskX context, and non-/mnt paths.
133-157: Directory and file listing logic is properly structured.The refactoring to pre-populate
$dirsand$filesarrays before rendering improves code organization. The parent directory navigation, exclusions, and match filtering are handled correctly.emhttp/plugins/dynamix/nchan/file_manager (8)
24-26: Constants for progress estimation are well-defined.Using constants for
RSYNC_MIN_PROGRESS_PERCENT(3%) andRSYNC_TOTAL_SIZE_SAMPLES(5) makes the algorithm configurable and self-documenting.
91-100: Thesize_to_bytesfunction may return incorrect results for certain inputs.The function uses
striposto find unit characters, but this could match units within the numeric portion (though unlikely). More importantly, if the input contains no unit, it returns just the float value, which is correct for bytes.However, the function doesn't handle lowercase single-character units consistently with the uppercase check on line 95. The
striposis case-insensitive, so "1.5g" would match, but the comparison uses uppercase keys.The implementation correctly uses
stripos(case-insensitive), which matches both "G" and "g" against the uppercase key 'G' in the multipliers array. This works because we're checking if the unit character exists in the string, not comparing the case of the unit directly.
109-121: Division safety inbytes_to_size.The function safely handles the case where
$bytesis 0 or very small by only dividing when$bytes >= $divisor. The default case returns "0.00B" for zero bytes, which is appropriate.
161-185: Docblock and reset mechanism properly implemented.The docblock clearly documents the function's purpose, parameters, and the static state reset behavior. The reset mechanism at the start of new operations (called on lines 438 and 482) ensures clean state between transfers.
207-232: Total size estimation algorithm is clever but has inherent inaccuracy.The algorithm averages multiple measurements to compensate for rsync's truncated percentage display (~2% error per point). The comment on line 222-223 adds 0.5% to account for truncation, which is a reasonable heuristic.
Note that this produces an estimate (prefixed with "~" in the output on line 252), which correctly sets user expectations.
366-376: JSON parse error logging properly implemented.The error logging for failed JSON parsing was addressed per previous review. The implementation logs the first 100 characters of the problematic JSON for debugging while avoiding log spam for empty files.
438-438: Static state reset calls placed correctly.The
parse_rsync_progress(null, null, true)calls are placed immediately before starting new copy/move operations, ensuring clean state for each transfer's progress tracking.Also applies to: 482-482
448-448: Error messages properly wrapped for translation.The hardcoded English error strings are now wrapped with
_()for localization, addressing the issue #2500 objective.Also applies to: 549-549, 579-579
| $rows = explode(',',$_POST['row']); | ||
| $lines = file($jobs, FILE_IGNORE_NEW_LINES); | ||
| foreach ($rows as $row) { | ||
| $end = $row + 8; | ||
| exec("sed -i '$row,$end d' $jobs"); | ||
| $line_number = $row - 1; // Convert 1-based job number to 0-based array index | ||
| if (isset($lines[$line_number])) { | ||
| unset($lines[$line_number]); | ||
| } | ||
| } | ||
| if (count($lines) > 0) { | ||
| file_put_contents($jobs, implode("\n", $lines)."\n"); | ||
| $undo = '2'; | ||
| } else { | ||
| delete_file($jobs); | ||
| $undo = '1'; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how the undo case is called from the frontend
rg -n "mode.*undo" --type=php -A5 -B2 emhttp/plugins/dynamix/Repository: unraid/webgui
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Search for input validation or sanitization of $_POST['row'] in Control.php
rg -n "\$_POST\['row'\]" --type=php -B5 -A10 emhttp/plugins/dynamix/Repository: unraid/webgui
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check if there are any numeric validation functions used in Control.php
rg -n "is_numeric|filter_var|ctype_digit|intval" --type=php emhttp/plugins/dynamix/include/Control.phpRepository: unraid/webgui
Length of output: 101
🏁 Script executed:
#!/bin/bash
# Get broader context of the undo functionality in Control.php
rg -n "undo\s*=" --type=php -B10 -A5 emhttp/plugins/dynamix/include/Control.phpRepository: unraid/webgui
Length of output: 938
🏁 Script executed:
#!/bin/bash
# Search for where the 'undo' mode is triggered from frontend
rg -n "undo" --type=html --type=js emhttp/plugins/dynamix/ -B2 -A2Repository: unraid/webgui
Length of output: 50371
🏁 Script executed:
#!/bin/bash
# Check if there's any validation of row parameter earlier in the script
head -100 emhttp/plugins/dynamix/include/Control.php | rg -n "row|POST"Repository: unraid/webgui
Length of output: 493
Validate $_POST['row'] as numeric before arithmetic operations.
The 1-based to 0-based conversion is correct, but $_POST['row'] values are used directly in arithmetic without validation. Non-numeric input will silently convert to 0 in PHP, potentially causing unintended array index modifications. Add validation such as is_numeric() or cast to int for each element in $rows before the calculation.
🤖 Prompt for AI Agents
In emhttp/plugins/dynamix/include/Control.php around lines 177 to 191,
$_POST['row'] values are used directly for 1-based to 0-based index conversion
which can produce unintended 0 or invalid indices for non-numeric input;
validate and sanitize each element of $rows before arithmetic by checking
isset($_POST['row']) and iterating over explode(',', $_POST['row']) while using
is_numeric() (or casting to int after validation), skip any non-numeric or
out-of-range values, convert validated 1-based ints to 0-based indices, and only
then unset $lines[$line_number]; also ensure row numbers are >=1 and within
count($lines) to avoid accidental deletions.
This addresses remaining feedback items and UI bugs:
Popular Destinations Context Fix:
- Add SHOW_POPULAR filter to copy/move operations (cases 3,4,8,9 in doAction)
- Add SHOW_POPULAR,HIDE_FILES_FILTER to bulk copy/move (doActions cases 3,4)
- FileTree.php now checks in_array('SHOW_POPULAR', $filters) instead of always showing
- Prevents Popular from appearing in Docker path selection and other contexts
- Works seamlessly with existing filter[] POST mechanism (no jquery.filetree.js changes)
Variable Conflict Resolution:
- Rename $root to $fileTreeRoot throughout FileTree.php
- Fixes conflict with Translations.php which overwrites $root variable (lines 128/140/176)
- Add Translations.php include for _('Popular') translation support
Display Improvements:
- Show full paths in Popular destinations instead of basename for clarity
- Add ui-corner-all class to all 4 File Manager dialogs for rounded corners
- Fix CSS: replace hardcoded unraid#888 with var(--alt-text-color) in default-dynamix.css
- Change all 6 margin-bottom from '20vh' to '320px' for consistent spacing
Code Cleanup:
- Remove unused $autocomplete variable (never set as POST parameter)
- Update comments to reflect actual behavior
Result: Popular destinations only appear in File Manager operations, not in Docker
or other file pickers. All dialogs now have consistent rounded corners.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
emhttp/plugins/dynamix/include/FileTree.php (1)
97-104: Escape translated text and use ENT_QUOTES for HTML attributes.Line 97 outputs
_('Popular')without HTML escaping, and line 100 useshtmlspecialchars()withoutENT_QUOTES. Since line 104 uses single-quoted HTML attributes (data-path='$htmlPath'), paths containing single quotes will break the attribute.🔎 Proposed fix
- echo "<li class='popular-header small-caps-label' style='list-style:none;padding:5px 0 5px 20px;'>"._('Popular')."</li>"; + echo "<li class='popular-header small-caps-label' style='list-style:none;padding:5px 0 5px 20px;'>".htmlspecialchars(_('Popular'), ENT_QUOTES, 'UTF-8')."</li>"; foreach ($popularPaths as $path) { - $htmlPath = htmlspecialchars($path); - $displayPath = htmlspecialchars($path); // Show full path instead of basename + $htmlPath = htmlspecialchars($path, ENT_QUOTES, 'UTF-8'); + $displayPath = htmlspecialchars($path, ENT_QUOTES, 'UTF-8'); // Use data-path instead of rel to prevent jQueryFileTree from handling these links // Use 'directory' class so jQueryFileTree CSS handles the icon echo "<li class='directory popular-destination' style='list-style:none;'>$checkbox<a href='#' data-path='$htmlPath'>$displayPath</a></li>";Based on learnings, translation should be applied first, then escaped with htmlspecialchars() to ensure proper handling of translation placeholders.
🧹 Nitpick comments (2)
emhttp/plugins/dynamix/include/FileTree.php (1)
144-157: Consider using ENT_QUOTES consistently for all htmlspecialchars() calls.While lines 144, 145, 151, and 152 correctly escape output with
htmlspecialchars(), they don't specify theENT_QUOTESflag. For consistency with security best practices and to handle all edge cases (even though these use double-quoted attributes), consider explicitly specifying the flags.🔎 Proposed refactor
- $htmlRel = htmlspecialchars(my_dir($name).$name); - $htmlName = htmlspecialchars(mb_strlen($name) <= 33 ? $name : mb_substr($name, 0, 30).'...'); + $htmlRel = htmlspecialchars(my_dir($name).$name, ENT_QUOTES, 'UTF-8'); + $htmlName = htmlspecialchars(mb_strlen($name) <= 33 ? $name : mb_substr($name, 0, 30).'...', ENT_QUOTES, 'UTF-8');Apply the same pattern to lines 151-152 for file entries.
emhttp/plugins/dynamix/Browse.page (1)
358-522: Consider refactoring this large function into smaller, focused functions.The 164-line
setupTargetNavigation()function handles multiple concerns: event prevention, popular destination handling, tree navigation, input handling, backward traversal, and cleanup. While the implementation is correct and functional, breaking it into smaller helper functions would improve maintainability and testability.Potential structure:
attachPreventCloseHandlers()handlePopularDestinationClick()handleTreeItemClick()handleInputNavigation()cleanupNavigationHandlers()
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
emhttp/plugins/dynamix/Browse.pageemhttp/plugins/dynamix/include/FileTree.phpemhttp/plugins/dynamix/styles/default-dynamix.css
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-09-05T19:26:36.587Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2354
File: emhttp/plugins/dynamix/ShareEdit.page:0-0
Timestamp: 2025-09-05T19:26:36.587Z
Learning: In emhttp/plugins/dynamix/ShareEdit.page, the clone-settings div was moved outside the form element and both are wrapped in a div.relative container to prevent event bubbling issues while maintaining proper positioning.
Applied to files:
emhttp/plugins/dynamix/styles/default-dynamix.css
📚 Learning: 2025-12-30T20:28:06.818Z
Learnt from: mgutt
Repo: unraid/webgui PR: 2501
File: emhttp/plugins/dynamix/Browse.page:1282-1339
Timestamp: 2025-12-30T20:28:06.818Z
Learning: In emhttp/plugins/dynamix/Browse.page, the xlink() function's clipboard functionality is intentionally designed to silently fail on HTTP connections without showing warnings. The path text is pre-selected to allow manual copying, which is preferred over showing repetitive error messages that would annoy HTTP users.
Applied to files:
emhttp/plugins/dynamix/Browse.page
📚 Learning: 2025-05-31T00:04:34.204Z
Learnt from: zackspear
Repo: unraid/webgui PR: 0
File: :0-0
Timestamp: 2025-05-31T00:04:34.204Z
Learning: For the unraid/webgui legacy codebase, ARIA labels and accessibility concerns should not be addressed in individual PRs. These will be handled via a different solution in the future.
Applied to files:
emhttp/plugins/dynamix/Browse.page
📚 Learning: 2025-08-13T03:13:53.283Z
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.
Applied to files:
emhttp/plugins/dynamix/Browse.page
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
Repo: unraid/webgui 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.
Applied to files:
emhttp/plugins/dynamix/include/FileTree.php
📚 Learning: 2025-02-01T15:12:16.914Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 1992
File: emhttp/plugins/dynamix/include/CPUset.php:0-0
Timestamp: 2025-02-01T15:12:16.914Z
Learning: In the unRAID WebGUI, when both translation and HTML escaping are needed, always apply translation (_()) first, then escape the translated text with htmlspecialchars() to ensure proper handling of translation placeholders.
Applied to files:
emhttp/plugins/dynamix/include/FileTree.php
📚 Learning: 2025-02-01T15:12:16.914Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 1992
File: emhttp/plugins/dynamix/include/CPUset.php:0-0
Timestamp: 2025-02-01T15:12:16.914Z
Learning: In the unRAID WebGUI, button text should be wrapped in _() for translation support and htmlspecialchars() for security, with a preference for using ternary operators for simple conditional text assignments.
Applied to files:
emhttp/plugins/dynamix/include/FileTree.php
📚 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/include/FileTree.php
🧬 Code graph analysis (1)
emhttp/plugins/dynamix/include/FileTree.php (1)
emhttp/plugins/dynamix/include/PopularDestinations.php (1)
getPopularDestinations(132-147)
🔇 Additional comments (9)
emhttp/plugins/dynamix/styles/default-dynamix.css (3)
1487-1487: LGTM: Improved dialog compactness.Reducing the minimum dialog height from 35vh to 20vh improves the user experience by making dialogs more compact, especially for simple dialogs with minimal content. This aligns well with the PR's objectives to enhance dialog layout.
1754-1770: LGTM: Appropriate responsive behavior for mobile devices.The responsive adjustments ensure that warning messages and buttons display properly on smaller screens by:
- Stacking the warning above the buttons (full-width block display)
- Centering content for better visual hierarchy
- Adjusting margins for appropriate spacing
This follows mobile-first design best practices and prevents layout issues on narrow viewports.
1501-1506: LGTM: Well-structured warning display styling with proper responsive design.The
.dfm-warningclass is properly implemented with appropriate positioning and styling to display inline warnings beside dialog buttons. Usingvar(--alt-text-color)maintains consistency with the existing design system. The base definition at line 1501 is paired with a responsive media query override at line 1757 (max-width: 768px), which properly adjusts the layout for smaller screens—this is appropriate CSS structure, not a duplicate.emhttp/plugins/dynamix/Browse.page (6)
60-66: LGTM!The function correctly prevents the file tree dropdown from closing by stopping event propagation. Good use of namespaced event handlers for proper cleanup.
524-618: LGTM!These four navigation functions are well-structured and work together cohesively to provide programmatic file tree navigation. Good use of constants (
FOLDER_EXPAND_DELAY,NAVIGATION_BUFFER) and jQuery.data() for state management.
750-773: LGTM!The new dialog
openhandler effectively adds contextual warnings to action dialogs. The warning texts are properly translated server-side and clearly inform users about the operation consequences.
884-889: LGTM!The integration of
preventFileTreeClose()and conditionalsetupTargetNavigation()for copy/move operations is clean and well-structured. The action filtering[3,4,8,9].includes(action)correctly identifies operations that require file tree navigation.
750-752: LGTM!The dialog
closehandler properly cleans up the namespacedmousedown.dfmFileTreeevent handlers, preventing memory leaks. The pattern is consistently applied across all relevant dialogs.
1280-1337: openTerminal() function signature correctly supports path parameter.The
openTerminal()function (defined inemhttp/plugins/dynamix/include/DefaultPageLayout/HeadInlineJS.phpat line 158) accepts three parameters:tag,name, andmore. The call at line 1298 correctly passes the path as the thirdmoreparameter, which is then forwarded to/webGui/include/OpenTerminal.phpvia GET request. The implementation properly chains the path through to the backend PHP script that sets the working directory for the terminal session.
| dfm.window.find('#dfm_target').attr('data-pickroot',root).attr('data-picktop',root).attr('data-pickmatch',match).attr('data-show-popular','true').fileTreeAttach(null,null,function(path){ | ||
| var bits = path.substr(1).split('/'); | ||
| var auto = bits.length>3 ? '' : share; | ||
| dfm.window.find('#dfm_target').val(path+auto).change(); | ||
| }); | ||
| dfm.height = 630; | ||
| dfm.window.find('#dfm_target').css('margin-bottom', '320px'); | ||
| break; | ||
| case 4: // move folder | ||
| dfm.window.html($('#dfm_templateMoveFolder').html()); | ||
| dfm_createSource(source.dfm_strip()); | ||
| dfm.window.find('#dfm_target').attr('data-pickroot',root).attr('data-picktop',root).attr('data-pickmatch',match).fileTreeAttach(null,null,function(path){ | ||
| dfm.window.find('#dfm_target').attr('data-pickroot',root).attr('data-picktop',root).attr('data-pickmatch',match).attr('data-pickfilter','SHOW_POPULAR').fileTreeAttach(null,null,function(path){ | ||
| var bits = path.substr(1).split('/'); | ||
| var auto = bits.length>3 ? '' : share; | ||
| dfm.window.find('#dfm_target').val(path+auto).change(); | ||
| }); | ||
| dfm.height = 630; | ||
| dfm.window.find('#dfm_target').css('margin-bottom', '320px'); | ||
| break; | ||
| case 6: // delete file | ||
| dfm.window.html($('#dfm_templateDeleteFile').html()); | ||
| dfm_createSource(source); | ||
| dfm.height = 330; | ||
| break; | ||
| case 7: // rename file | ||
| dfm.window.html($('#dfm_templateRenameFile').html()); | ||
| dfm_createSource(name); | ||
| dfm.window.find('#dfm_target').val(name); | ||
| dfm.height = 330; | ||
| break; | ||
| case 8: // copy file | ||
| dfm.window.html($('#dfm_templateCopyFile').html()); | ||
| dfm_createSource(source); | ||
| dfm.window.find('#dfm_target').attr('data-pickroot',root).attr('data-picktop',root).attr('data-pickmatch',match).fileTreeAttach(null,null,function(path){ | ||
| dfm.window.find('#dfm_target').attr('data-pickroot',root).attr('data-picktop',root).attr('data-pickmatch',match).attr('data-pickfilter','SHOW_POPULAR').fileTreeAttach(null,null,function(path){ | ||
| dfm.window.find('#dfm_target').val(path).change(); | ||
| }); | ||
| dfm.height = 630; | ||
| dfm.window.find('#dfm_target').css('margin-bottom', '320px'); | ||
| break; | ||
| case 9: // move file | ||
| dfm.window.html($('#dfm_templateMoveFile').html()); | ||
| dfm_createSource(source); | ||
| dfm.window.find('#dfm_target').attr('data-pickroot',root).attr('data-picktop',root).attr('data-pickmatch',match).fileTreeAttach(null,null,function(path){ | ||
| dfm.window.find('#dfm_target').attr('data-pickroot',root).attr('data-picktop',root).attr('data-pickmatch',match).attr('data-pickfilter','SHOW_POPULAR').fileTreeAttach(null,null,function(path){ | ||
| dfm.window.find('#dfm_target').val(path).change(); | ||
| }); | ||
| dfm.height = 630; | ||
| dfm.window.find('#dfm_target').css('margin-bottom', '320px'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file exists and get an overview
ls -lh emhttp/plugins/dynamix/Browse.page
wc -l emhttp/plugins/dynamix/Browse.pageRepository: unraid/webgui
Length of output: 179
🏁 Script executed:
# Search for all occurrences of margin-bottom and height-related CSS in Browse.page
rg -n "margin-bottom|\.css\('height'|\.css\(\"height\"" emhttp/plugins/dynamix/Browse.page -B 2 -A 1 | head -100Repository: unraid/webgui
Length of output: 1039
🏁 Script executed:
# Search for FileTree-related code and its height/sizing
rg -n "fileTreeAttach|FileTree|file.tree" emhttp/plugins/dynamix/Browse.page -B 3 -A 3 | head -150Repository: unraid/webgui
Length of output: 5801
🏁 Script executed:
# Check if 320px appears elsewhere in the file and if there are any responsive/dynamic height calculations
rg -n "320px|height.*\(|margin.*function|getHeight|scrollHeight" emhttp/plugins/dynamix/Browse.page | head -50Repository: unraid/webgui
Length of output: 997
Address fixed 320px margin-bottom value in dialog file pickers.
The margin-bottom values at lines 669, 679, 696, 704, 982, and 995 use a hardcoded 320px to prevent FileTree overlap. While this ensures spacing, the fixed value doesn't adapt to viewport dimensions or container heights. Given that other dialog sizing in the file uses responsive calculations (e.g., line 290: Math.min(window.innerHeight-80,800)), consider:
- Measuring the actual FileTree height dynamically and adjusting margin accordingly
- Using CSS
max-heightor container constraints instead of fixed margins - Testing the current spacing on mobile/small screens to confirm usability
1 participant
This PR addresses multiple issues and feature requests:
Browse.pageExample:
Create-Dialog is smaller because it does not have the margin:
Totalduring operationsTERMINALbuttonmoreGET param inOpenTerminal.phpas path parameter/rootas$HOMEdirPopularDestinations.phpwith frequency-based scoring system/boot/config/filemanager.json(will be re-used for future persistent FileManager settings)Example:
filemanager.json:/path/into the target input field, automatically "opens" the visual representation of this directory in the FileTree browsersetupTargetNavigation()functionExample:
filetree-upgrade.mp4
Additional fixes:
Fixes #2500
Dependencies
This PR depends on the following PRs being merged first:
Summary by CodeRabbit
New Features
UI Improvements
Stability
✏️ Tip: You can customize this high-level summary in your review settings.