From 81a022dbe8d5e51952e0b17d8975c9cd9d85999c Mon Sep 17 00:00:00 2001 From: 5ymb01 <5ymb01ixm@gmail.com> Date: Wed, 25 Mar 2026 12:57:04 -0400 Subject: [PATCH] fix(web): resolve file upload config lookup for server-rendered forms (#279) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(web): resolve file upload config lookup for server-rendered forms The file upload widget's getUploadConfig() function failed to map server-rendered field IDs (e.g., "static-image-images") back to schema property keys ("images"), causing upload config (plugin_id, endpoint, allowed_types) to be lost. This could prevent image uploads from working correctly in the static-image plugin and others. Changes: - Add data-* attributes to the Jinja2 file-upload template so upload config is embedded directly on the file input element - Update getUploadConfig() in both file-upload.js and plugins_manager.js to read config from data attributes first, falling back to schema lookup - Remove duplicate handleFiles/handleFileDrop/handleFileSelect from plugins_manager.js that overwrote the more robust file-upload.js versions - Bump cache-busting version strings so browsers fetch updated JS Co-Authored-By: Claude Opus 4.6 * fix(web): harden file upload functions against CodeRabbit patterns - Add response.ok guard before response.json() in handleFiles, deleteUploadedFile, and handleCredentialsUpload to prevent SyntaxError on non-JSON error responses (PR #271 finding) - Remove duplicate getUploadConfig() from plugins_manager.js; file-upload.js now owns this function exclusively - Replace innerHTML with textContent/DOM methods in handleCredentialsUpload to prevent XSS (PR #271 finding) - Fix redundant if-check in getUploadConfig data-attribute reader Co-Authored-By: Claude Opus 4.6 * fix(web): address CodeRabbit findings on file upload widget - Add data-multiple="true" discriminator on array file inputs so handleFileDrop routes multi-file drops to handleFiles() not handleSingleFileUpload() - Duplicate upload config data attributes onto drop zone wrapper so getUploadConfig() survives progress-helper DOM re-renders that remove the file input element - Clear file input in finally block after credentials upload to allow re-selecting the same file on retry - Branch deleteUploadedFile on fileType: JSON deletes remove the DOM element directly instead of routing through updateImageList() which renders image-specific cards (thumbnails, scheduling controls) Addresses CodeRabbit findings on PR #279: - Major: drag-and-drop hits single-file path for array uploaders - Major: config lookup fails after first upload (DOM node removed) - Minor: same-file retry silently no-ops - Major: JSON deletes re-render list as images Co-Authored-By: 5ymb01 <5ymb01@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 * fix(web): address CodeRabbit round-2 findings on file upload widget - Extract getConfigSourceElement() helper so handleFileDrop, handleSingleFileUpload, and getUploadConfig all share the same fallback logic: file input → drop zone wrapper - Remove pluginId gate from getUploadConfig Strategy 1 — fields with uploadEndpoint or fileType but no pluginId now return config instead of falling through to generic defaults - Fix JSON delete identifier mismatch: use file.id || file.category_name (matching the renderer at line 3202) instead of f.file_id; remove regex sanitization on DOM id lookup (renderer doesn't sanitize) Addresses CodeRabbit round-2 findings on PR #279: - Major: single-file uploads bypass drop-zone config fallback - Major: getUploadConfig gated on data-plugin-id only - Major: JSON delete file identifier mismatch vs renderer Co-Authored-By: 5ymb01 <5ymb01@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 * fix(web): align delete handler file identifier with renderer logic Remove f.file_id from JSON file delete filter to match the renderer's identifier logic (file.id || file.category_name || idx). Prevents deleted entries from persisting in the hidden input on next save. Co-Authored-By: 5ymb01 Co-Authored-By: Claude Opus 4.6 --------- Co-authored-by: 5ymb01 <5ymb01@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 Co-authored-by: 5ymb01 --- .../static/v3/js/widgets/file-upload.js | 99 ++++++-- web_interface/static/v3/plugins_manager.js | 214 +++++------------- web_interface/templates/v3/base.html | 4 +- .../templates/v3/partials/plugin_config.html | 25 +- 4 files changed, 155 insertions(+), 187 deletions(-) diff --git a/web_interface/static/v3/js/widgets/file-upload.js b/web_interface/static/v3/js/widgets/file-upload.js index 846dc78a..0d553235 100644 --- a/web_interface/static/v3/js/widgets/file-upload.js +++ b/web_interface/static/v3/js/widgets/file-upload.js @@ -72,9 +72,10 @@ event.preventDefault(); const files = event.dataTransfer.files; if (files.length === 0) return; - // Route to single-file handler if this is a string file-upload widget - const fileInput = document.getElementById(`${fieldId}_file_input`); - if (fileInput && fileInput.dataset.uploadEndpoint && fileInput.dataset.uploadEndpoint.trim() !== '') { + // Route to single-file handler only for non-multiple string file-upload widgets + const configEl = getConfigSourceElement(fieldId); + const isMultiple = configEl && configEl.dataset.multiple === 'true'; + if (!isMultiple && configEl && configEl.dataset.uploadEndpoint && configEl.dataset.uploadEndpoint.trim() !== '') { window.handleSingleFileUpload(fieldId, files[0]); } else { window.handleFiles(fieldId, Array.from(files)); @@ -111,14 +112,33 @@ * @param {string} fieldId - Field ID * @param {File} file - File to upload */ - window.handleSingleFileUpload = async function(fieldId, file) { + /** + * Resolve the config source element for a field, checking file input first + * then falling back to the drop zone wrapper (which survives re-renders). + * @param {string} fieldId - Field ID + * @returns {HTMLElement|null} Element with data attributes, or null + */ + function getConfigSourceElement(fieldId) { const fileInput = document.getElementById(`${fieldId}_file_input`); - if (!fileInput) return; + if (fileInput && (fileInput.dataset.pluginId || fileInput.dataset.uploadEndpoint)) { + return fileInput; + } + const dropZone = document.getElementById(`${fieldId}_drop_zone`); + if (dropZone && (dropZone.dataset.pluginId || dropZone.dataset.uploadEndpoint)) { + return dropZone; + } + return null; + } - const uploadEndpoint = fileInput.dataset.uploadEndpoint; - const targetFilename = fileInput.dataset.targetFilename || 'file.json'; - const maxSizeMB = parseFloat(fileInput.dataset.maxSizeMb || '1'); - const allowedExtensions = (fileInput.dataset.allowedExtensions || '.json') + window.handleSingleFileUpload = async function(fieldId, file) { + // Read config from file input or drop zone fallback (survives re-renders) + const configEl = getConfigSourceElement(fieldId); + if (!configEl) return; + + const uploadEndpoint = configEl.dataset.uploadEndpoint; + const targetFilename = configEl.dataset.targetFilename || 'file.json'; + const maxSizeMB = parseFloat(configEl.dataset.maxSizeMb || '1'); + const allowedExtensions = (configEl.dataset.allowedExtensions || '.json') .split(',').map(e => e.trim().toLowerCase()); const statusDiv = document.getElementById(`${fieldId}_upload_status`); @@ -280,9 +300,14 @@ method: 'POST', body: formData }); - + + if (!response.ok) { + const body = await response.text(); + throw new Error(`Server error ${response.status}: ${body}`); + } + const data = await response.json(); - + if (data.status === 'success') { // Add uploaded files to current list const currentFiles = window.getCurrentImages ? window.getCurrentImages(fieldId) : []; @@ -348,9 +373,14 @@ headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(requestBody) }); - + + if (!response.ok) { + const body = await response.text(); + throw new Error(`Server error ${response.status}: ${body}`); + } + const data = await response.json(); - + if (data.status === 'success') { // Remove from current list - normalize types for comparison const currentFiles = window.getCurrentImages ? window.getCurrentImages(fieldId) : []; @@ -377,21 +407,42 @@ }; /** - * Get upload configuration from schema + * Get upload configuration for a file upload field. + * Priority: 1) data attributes on the file input element (server-rendered), + * 2) schema lookup via window.currentPluginConfig (client-rendered). * @param {string} fieldId - Field ID * @returns {Object} Upload configuration */ window.getUploadConfig = function(fieldId) { - // Extract config from schema + // Strategy 1: Read from data attributes on the file input element or + // the drop zone wrapper (which survives progress-helper re-renders). + // Accept any upload-related data attribute — not just pluginId. + const configSource = getConfigSourceElement(fieldId); + if (configSource) { + const ds = configSource.dataset; + const config = {}; + if (ds.pluginId) config.plugin_id = ds.pluginId; + if (ds.uploadEndpoint) config.endpoint = ds.uploadEndpoint; + if (ds.fileType) config.file_type = ds.fileType; + if (ds.maxFiles) config.max_files = parseInt(ds.maxFiles, 10); + if (ds.maxSizeMb) config.max_size_mb = parseFloat(ds.maxSizeMb); + if (ds.allowedTypes) { + config.allowed_types = ds.allowedTypes.split(',').map(t => t.trim()); + } + return config; + } + + // Strategy 2: Extract config from schema (client-side rendered forms) const schema = window.currentPluginConfig?.schema; if (!schema || !schema.properties) return {}; - + // Find the property that matches this fieldId - // FieldId is like "image_config_images" for "image_config.images" + // FieldId is like "image_config_images" for "image_config.images" (client-side) + // or "static-image-images" for plugin "static-image", field "images" (server-side) const key = fieldId.replace(/_/g, '.'); const keys = key.split('.'); let prop = schema.properties; - + for (const k of keys) { if (prop && prop[k]) { prop = prop[k]; @@ -404,22 +455,22 @@ } } } - + // If we found an array with x-widget, get its config if (prop && prop.type === 'array' && prop['x-widget'] === 'file-upload') { return prop['x-upload-config'] || {}; } - - // Try to find nested images array - if (schema.properties && schema.properties.image_config && - schema.properties.image_config.properties && + + // Try to find nested images array (legacy fallback) + if (schema.properties && schema.properties.image_config && + schema.properties.image_config.properties && schema.properties.image_config.properties.images) { const imagesProp = schema.properties.image_config.properties.images; if (imagesProp['x-widget'] === 'file-upload') { return imagesProp['x-upload-config'] || {}; } } - + return {}; }; diff --git a/web_interface/static/v3/plugins_manager.js b/web_interface/static/v3/plugins_manager.js index d122d5fe..549d58ce 100644 --- a/web_interface/static/v3/plugins_manager.js +++ b/web_interface/static/v3/plugins_manager.js @@ -6622,22 +6622,9 @@ window.closeInstructionsModal = function() { } // ==================== File Upload Functions ==================== -// Make these globally accessible for use in base.html - -window.handleFileDrop = function(event, fieldId) { - event.preventDefault(); - const files = event.dataTransfer.files; - if (files.length > 0) { - window.handleFiles(fieldId, Array.from(files)); - } -} - -window.handleFileSelect = function(event, fieldId) { - const files = event.target.files; - if (files.length > 0) { - window.handleFiles(fieldId, Array.from(files)); - } -} +// Note: handleFileDrop, handleFileSelect, and handleFiles are defined in +// file-upload.js widget which loads first. We only define supplementary +// functions here that file-upload.js doesn't provide. window.handleCredentialsUpload = async function(event, fieldId, uploadEndpoint, targetFilename) { const file = event.target.files[0]; @@ -6661,7 +6648,11 @@ window.handleCredentialsUpload = async function(event, fieldId, uploadEndpoint, // Show upload status const statusEl = document.getElementById(fieldId + '_status'); if (statusEl) { - statusEl.innerHTML = 'Uploading...'; + statusEl.textContent = ''; + const spinner = document.createElement('i'); + spinner.className = 'fas fa-spinner fa-spin mr-2'; + statusEl.appendChild(spinner); + statusEl.appendChild(document.createTextNode('Uploading...')); } // Create form data @@ -6673,9 +6664,14 @@ window.handleCredentialsUpload = async function(event, fieldId, uploadEndpoint, method: 'POST', body: formData }); - + + if (!response.ok) { + const body = await response.text(); + throw new Error(`Server error ${response.status}: ${body}`); + } + const data = await response.json(); - + if (data.status === 'success') { // Update hidden input with filename const hiddenInput = document.getElementById(fieldId + '_hidden'); @@ -6685,112 +6681,31 @@ window.handleCredentialsUpload = async function(event, fieldId, uploadEndpoint, // Update status if (statusEl) { - statusEl.innerHTML = `✓ Uploaded: ${targetFilename || file.name}`; + statusEl.textContent = `✓ Uploaded: ${targetFilename || file.name}`; statusEl.className = 'text-sm text-green-600'; } showNotification('Credentials file uploaded successfully', 'success'); } else { if (statusEl) { - statusEl.innerHTML = 'Upload failed - click to try again'; + statusEl.textContent = 'Upload failed - click to try again'; statusEl.className = 'text-sm text-gray-600'; } showNotification(data.message || 'Upload failed', 'error'); } } catch (error) { if (statusEl) { - statusEl.innerHTML = 'Upload failed - click to try again'; + statusEl.textContent = 'Upload failed - click to try again'; statusEl.className = 'text-sm text-gray-600'; } showNotification('Error uploading file: ' + error.message, 'error'); + } finally { + // Allow re-selecting the same file on the next attempt + event.target.value = ''; } } -window.handleFiles = async function(fieldId, files) { - const uploadConfig = window.getUploadConfig(fieldId); - const pluginId = uploadConfig.plugin_id || window.currentPluginConfig?.pluginId || 'static-image'; - const maxFiles = uploadConfig.max_files || 10; - const maxSizeMB = uploadConfig.max_size_mb || 5; - const fileType = uploadConfig.file_type || 'image'; - const customUploadEndpoint = uploadConfig.endpoint || '/api/v3/plugins/assets/upload'; - - // Get current files list (works for both images and JSON) - const currentFiles = window.getCurrentImages ? window.getCurrentImages(fieldId) : []; - if (currentFiles.length + files.length > maxFiles) { - showNotification(`Maximum ${maxFiles} files allowed. You have ${currentFiles.length} and tried to add ${files.length}.`, 'error'); - return; - } - - // Validate file types and sizes - const validFiles = []; - for (const file of files) { - if (file.size > maxSizeMB * 1024 * 1024) { - showNotification(`File ${file.name} exceeds ${maxSizeMB}MB limit`, 'error'); - continue; - } - - if (fileType === 'json') { - // Validate JSON files - if (!file.name.toLowerCase().endsWith('.json')) { - showNotification(`File ${file.name} must be a JSON file (.json)`, 'error'); - continue; - } - } else { - // Validate image files - const allowedTypes = ['image/png', 'image/jpeg', 'image/jpg', 'image/bmp', 'image/gif']; - if (!allowedTypes.includes(file.type)) { - showNotification(`File ${file.name} is not a valid image type`, 'error'); - continue; - } - } - - validFiles.push(file); - } - - if (validFiles.length === 0) { - return; - } - - // Show upload progress - window.showUploadProgress(fieldId, validFiles.length); - - // Upload files - const formData = new FormData(); - if (fileType !== 'json') { - formData.append('plugin_id', pluginId); - } - validFiles.forEach(file => formData.append('files', file)); - - try { - const response = await fetch(customUploadEndpoint, { - method: 'POST', - body: formData - }); - - const data = await response.json(); - - if (data.status === 'success') { - // Add uploaded files to current list - const currentFiles = window.getCurrentImages ? window.getCurrentImages(fieldId) : []; - const newFiles = [...currentFiles, ...data.uploaded_files]; - window.updateImageList(fieldId, newFiles); - - showNotification(`Successfully uploaded ${data.uploaded_files.length} ${fileType === 'json' ? 'file(s)' : 'image(s)'}`, 'success'); - } else { - showNotification(`Upload failed: ${data.message}`, 'error'); - } - } catch (error) { - console.error('Upload error:', error); - showNotification(`Upload error: ${error.message}`, 'error'); - } finally { - window.hideUploadProgress(fieldId); - // Clear file input - const fileInput = document.getElementById(`${fieldId}_file_input`); - if (fileInput) { - fileInput.value = ''; - } - } -} +// handleFiles is now defined exclusively in file-upload.js widget window.deleteUploadedImage = async function(fieldId, imageId, pluginId) { return window.deleteUploadedFile(fieldId, imageId, pluginId, 'image', null); @@ -6813,15 +6728,43 @@ window.deleteUploadedFile = async function(fieldId, fileId, pluginId, fileType, headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(requestBody) }); - + + if (!response.ok) { + const body = await response.text(); + throw new Error(`Server error ${response.status}: ${body}`); + } + const data = await response.json(); - + if (data.status === 'success') { - // Remove from current list - const currentFiles = window.getCurrentImages ? window.getCurrentImages(fieldId) : []; - const newFiles = currentFiles.filter(file => (file.id || file.category_name) !== fileId); - window.updateImageList(fieldId, newFiles); - + if (fileType === 'json') { + // For JSON files, remove the item's DOM element directly since + // updateImageList renders image-specific cards (thumbnails, scheduling). + const fileEl = document.getElementById(`file_${fileId}`); + if (fileEl) fileEl.remove(); + // Update hidden data input — normalize identifiers to strings + // since JSON files may use id, file_id, or category_name + const currentFiles = window.getCurrentImages ? window.getCurrentImages(fieldId) : []; + const fileIdStr = String(fileId); + const newFiles = currentFiles.filter(f => { + // Match the same identifier logic as the renderer: + // file.id || file.category_name || idx (see renderArrayField) + const fid = String(f.id || f.category_name || ''); + return fid !== fileIdStr; + }); + const hiddenInput = document.getElementById(`${fieldId}_images_data`); + if (hiddenInput) hiddenInput.value = JSON.stringify(newFiles); + } else { + // For images, use the full image list re-renderer — normalize to strings + const currentFiles = window.getCurrentImages ? window.getCurrentImages(fieldId) : []; + const fileIdStr = String(fileId); + const newFiles = currentFiles.filter(file => { + const fid = String(file.id || file.category_name || ''); + return fid !== fileIdStr; + }); + window.updateImageList(fieldId, newFiles); + } + showNotification(`${fileType === 'json' ? 'File' : 'Image'} deleted successfully`, 'success'); } else { showNotification(`Delete failed: ${data.message}`, 'error'); @@ -6832,47 +6775,8 @@ window.deleteUploadedFile = async function(fieldId, fileId, pluginId, fileType, } } -window.getUploadConfig = function(fieldId) { - // Extract config from schema - const schema = window.currentPluginConfig?.schema; - if (!schema || !schema.properties) return {}; - - // Find the property that matches this fieldId - // FieldId is like "image_config_images" for "image_config.images" - const key = fieldId.replace(/_/g, '.'); - const keys = key.split('.'); - let prop = schema.properties; - - for (const k of keys) { - if (prop && prop[k]) { - prop = prop[k]; - if (prop.properties && prop.type === 'object') { - prop = prop.properties; - } else if (prop.type === 'array' && prop['x-widget'] === 'file-upload') { - break; - } else { - break; - } - } - } - - // If we found an array with x-widget, get its config - if (prop && prop.type === 'array' && prop['x-widget'] === 'file-upload') { - return prop['x-upload-config'] || {}; - } - - // Try to find nested images array - if (schema.properties && schema.properties.image_config && - schema.properties.image_config.properties && - schema.properties.image_config.properties.images) { - const imagesProp = schema.properties.image_config.properties.images; - if (imagesProp['x-widget'] === 'file-upload') { - return imagesProp['x-upload-config'] || {}; - } - } - - return {}; -} +// getUploadConfig is defined in file-upload.js widget which loads first. +// No override needed here — file-upload.js owns this function. window.getCurrentImages = function(fieldId) { const hiddenInput = document.getElementById(`${fieldId}_images_data`); diff --git a/web_interface/templates/v3/base.html b/web_interface/templates/v3/base.html index 8e161a86..c1a5139e 100644 --- a/web_interface/templates/v3/base.html +++ b/web_interface/templates/v3/base.html @@ -4988,7 +4988,7 @@ - + @@ -5014,7 +5014,7 @@ - +