Fix #2514: Complete migration to JSON API for file_manager operations #2521
Conversation
…ations This PR completes the migration from HTML string responses to a unified JSON API format for all file_manager operations, building on the work started in PR unraid#2429. Backend changes (nchan/file_manager): - Migrate remaining actions to JSON format: * case 0 (create folder): {action: 0, text: ['Creating...']} * case 2/7 (rename): {action: 2/7, text: ['Renaming...']} * case 11 (chown): {action: 11, text: ['Updating... filename']} * case 12 (chmod): {action: 12, text: ['Updating... filename']} * case 15 (search): {action: 15, text: ['Searching... count']} * completion: {action: X, text: ['Done']} for all non-search operations - Convert search results to JSON structure: * {action: 15, results: [{location, path}, ...]} * Replaces legacy #cat# string format with structured data - Convert cat() function to return array of objects instead of string - Send errors as plain text (not HTML) for safer frontend handling Frontend changes (BrowseButton.page): - Implement unified JSON parser for all status updates - Show footer only for progress-tracking operations (delete, copy, move, search) - No footer for quick operations (create, rename, chown, chmod) - Parse search results from JSON structure: * Display as properly paired dt/dd elements * Clear previous results before showing new ones * Show 'No files found' for empty results - Remove all legacy string parsing code - Use .text() with white-space:pre-line for error display (prevents XSS) - Add error logging for JSON parse failures Frontend changes (Browse.page): - Add try-catch for Control.php JSON parsing - Handle empty responses gracefully All file_manager operations now use a consistent, type-safe JSON API with proper error handling and no legacy code paths remaining.
WalkthroughBrowse-related JavaScript now parses file-manager responses defensively; BrowseButton handles action 15 search results as JSON-first and improves multiline error display; file_manager PHP responses (including cat()) were migrated from plain/HTML text to structured JSON objects and arrays. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant BrowseJS as "Browse.page / BrowseButton.page (JS)"
participant WebSocket as "nchan WebSocket"
participant FileManager as "file_manager (PHP)"
Browser->>BrowseJS: trigger action or search
BrowseJS->>WebSocket: send request (action id, payload)
WebSocket->>FileManager: forward request
FileManager-->>WebSocket: stream JSON messages (progress/results/errors)
WebSocket-->>BrowseJS: deliver JSON payloads
BrowseJS->>BrowseJS: try { parsed = JSON.parse(data) } catch
alt parse succeeds
BrowseJS->>Browser: render using parsed.action / parsed.text / parsed.results
else parse fails
BrowseJS->>Console: log parse error
BrowseJS->>Browser: clear/replace progress UI (empty object handling)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🔇 Additional comments (10)
✏️ Tip: You can disable this entire section by setting 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: 1
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/nchan/file_manager (1)
298-331: Guard against missing location entries when building search results.
Ifgetfattrreturns fewer entries than$user,$set[++$i]can be undefined, yielding notices andnulllocations in JSON.💡 Suggested fix
- foreach ($user as $row) { - $results[] = [ - 'location' => $set[++$i], - 'path' => $row - ]; - } + foreach ($user as $row) { + $results[] = [ + 'location' => $set[++$i] ?? '---', + 'path' => $row + ]; + }
🤖 Fix all issues with AI agents
In `@emhttp/plugins/dynamix/BrowseButton.page`:
- Around line 155-175: When handling search results in the block where
parsed.action === 15 and parsed.results.length > 0, clear the progress text so
the previous "Searching…" message doesn't remain; call
dfm.window.find('.dfm_text').html('') (and optionally
dfm.window.find('.dfm_loc').html('') if appropriate) before or immediately after
appending the new results and updating `#dfm_files`, so the UI shows only the
fresh results; locate this change inside the same result-handling branch that
uses parsed.results, $dl, and dfm.window.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
emhttp/plugins/dynamix/Browse.pageemhttp/plugins/dynamix/BrowseButton.pageemhttp/plugins/dynamix/nchan/file_manager
🔇 Additional comments (5)
emhttp/plugins/dynamix/Browse.page (1)
877-883: Defensive JSON parsing is a solid safeguard.
This avoids hard failures on malformed Control.php responses and keeps the UI resilient.emhttp/plugins/dynamix/BrowseButton.page (3)
238-245: Footer display rules look consistent with the new action set.
251-253: JSON parse failure logging is a good safety net.
411-416:white-space: pre-lineimproves multi-line error readability.emhttp/plugins/dynamix/nchan/file_manager (1)
400-407: Unified JSON status payloads and plain-text error output fit the new API contract.Also applies to: 432-438, 632-638, 647-653, 662-668, 698-701, 704-707, 718-718
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Guard against undefined array access in cat() using null coalescing operator - Clear stale 'Searching...' text after search results render
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/nchan/file_manager (1)
419-429: The frontend incorrectly displays HTML-escaped text as literal entity codes.The backend applies
htmlspecialchars()to exec output (lines 422, 489, 637, 652, 262), encoding special characters into HTML entities. However, the frontend'sdfm_showProgress()function (BrowseButton.page:208, 215, 219, 225, 229) inserts this content usingdocument.createTextNode(), which treats the text as plain content without decoding HTML entities. Users will see literal<,&, etc. instead of the intended<,&characters.Either remove
htmlspecialchars()from the backend and rely ondocument.createTextNode()for safe plain-text insertion, or switch the frontend to use.innerHTMLto properly decode the pre-escaped content.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/dynamix/BrowseButton.pageemhttp/plugins/dynamix/nchan/file_manager
🚧 Files skipped from review as they are similar to previous changes (1)
- emhttp/plugins/dynamix/BrowseButton.page
🔇 Additional comments (6)
emhttp/plugins/dynamix/nchan/file_manager (6)
298-331: LGTM! Good defensive coding with null coalescing.The
cat()function correctly returns a structured array of objects. The use of$set[++$i] ?? '---'on line 325 properly guards against undefined array access when the xattr output doesn't contain the expected number of elements.
399-414: LGTM!The JSON response structure
{ action, text }is consistent with other actions, and the implementation correctly handles the quick create operation without progress tracking.
430-445: LGTM!The rename operation JSON response follows the established pattern consistently.
630-659: LGTM!The chown/chmod operations correctly extract filenames from verbose output and apply
htmlspecialchars()for XSS protection. The JSON response structure is consistent with other actions.
660-674: LGTM!The search progress feedback correctly extracts and displays the result count. The JSON structure is consistent with the established pattern.
697-718: LGTM! Dual completion structures for regular vs. search operations.The implementation correctly differentiates between:
- Regular operations:
{ action, text: ['Done'] }- Search operations:
{ action, results: [...] }with structured data fromcat()The error handling on line 718 correctly outputs plain text (trimmed), aligning with the PR objective to send errors as plain text rather than HTML.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Since the frontend uses createTextNode() which is already XSS-safe and doesn't decode HTML entities, the backend htmlspecialchars() encoding is unnecessary and causes literal entity codes to appear in the UI.
1 participant
This PR completes the migration from HTML string responses to a unified JSON API format for all file_manager operations, building on the work started in PR #2429.
Backend changes (nchan/file_manager):
Frontend changes (BrowseButton.page):
Frontend changes (Browse.page):
All file_manager operations now use a consistent, type-safe JSON API with proper error handling and no legacy code paths remaining.
Fixes #2514
Summary by CodeRabbit
Bug Fixes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.