Skip to content
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions src/extensions/core/load3d/Load3DConfiguration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,27 @@ class Load3DConfiguration {
setting.loadFolder,
setting.cameraState
)

if (setting.modelWidget.options?.values) {
let values = setting.modelWidget.options.values as string[]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add type guard before casting to string[].

The code casts setting.modelWidget.options.values to string[] without verifying that it's actually an array of strings. This could cause runtime errors if the values contain non-string elements.

🔎 Proposed fix with type guard
 if (setting.modelWidget.options?.values) {
-  let values = setting.modelWidget.options.values as string[]
+  const rawValues = setting.modelWidget.options.values
+  if (!Array.isArray(rawValues)) {
+    return
+  }
+  let values = rawValues.filter((v): v is string => typeof v === 'string')
🤖 Prompt for AI Agents
In src/extensions/core/load3d/Load3DConfiguration.ts around lines 43-44, the
code blindly casts setting.modelWidget.options.values to string[]; add a type
guard to ensure values is an array of strings before casting: check
Array.isArray(setting.modelWidget.options.values) and that every element has
typeof === "string" (or filter/map to coerce only string elements), then assign
to values; if the guard fails, handle gracefully (e.g., skip processing or
default to an empty array) instead of casting directly.


try {
const stored = localStorage.getItem('Comfy.Load3D.HiddenFiles')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Extract magic constants for maintainability.

The hard-coded localStorage key 'Comfy.Load3D.HiddenFiles' and the magic number 12 should be extracted as named constants at the class or module level for better maintainability and consistency.

🔎 Proposed refactor

Add constants at the top of the class:

class Load3DConfiguration {
  private static readonly HIDDEN_FILES_STORAGE_KEY = 'Comfy.Load3D.HiddenFiles'
  private static readonly MAX_MESH_OPTIONS = 12

  // ... rest of the class

Then use them in the configure method:

     try {
-      const stored = localStorage.getItem('Comfy.Load3D.HiddenFiles')
+      const stored = localStorage.getItem(Load3DConfiguration.HIDDEN_FILES_STORAGE_KEY)
       const hiddenFiles = stored ? JSON.parse(stored) : []
       if (hiddenFiles.length > 0) {
         values = values.filter((v) => !hiddenFiles.includes(v))
       }
     } catch (e) {
       console.error('Failed to read hidden files from localStorage', e)
     }

-    if (values.length > 12) {
-      values = values.slice(0, 12)
+    if (values.length > Load3DConfiguration.MAX_MESH_OPTIONS) {
+      values = values.slice(0, Load3DConfiguration.MAX_MESH_OPTIONS)
     }

Also applies to: 56-56

🤖 Prompt for AI Agents
In @src/extensions/core/load3d/Load3DConfiguration.ts around line 47, Extract
the magic values by adding named constants (e.g., HIDDEN_FILES_STORAGE_KEY and
MAX_MESH_OPTIONS) at the class or module level and replace the inline string
'Comfy.Load3D.HiddenFiles' and the numeric literal 12 with those constants;
update usages in Load3DConfiguration.configure (and any other occurrences in
this file) to reference Load3DConfiguration.HIDDEN_FILES_STORAGE_KEY and
Load3DConfiguration.MAX_MESH_OPTIONS (or module-scoped equivalents) for
maintainability and consistency.

const hiddenFiles = stored ? JSON.parse(stored) : []
if (hiddenFiles.length > 0) {
values = values.filter((v) => !hiddenFiles.includes(v))
}
} catch (e) {
console.error('Failed to read hidden files from localStorage', e)
}

if (values.length > 12) {
values = values.slice(0, 12)
}

setting.modelWidget.options.values = values
}
Comment on lines +43 to +61

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Add type guard before casting to string[].

The code casts setting.meshWidget.options.values to string[] without verifying that it's actually an array of strings. This could cause runtime errors if the values contain non-string elements.

🔎 Proposed fix with type guard
 if (setting.meshWidget.options?.values) {
-  let values = setting.meshWidget.options.values as string[]
+  const rawValues = setting.meshWidget.options.values
+  if (!Array.isArray(rawValues)) {
+    return
+  }
+  let values = rawValues.filter((v): v is string => typeof v === 'string')
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (setting.meshWidget.options?.values) {
let values = setting.meshWidget.options.values as string[]
try {
const stored = localStorage.getItem('Comfy.Load3D.HiddenFiles')
const hiddenFiles = stored ? JSON.parse(stored) : []
if (hiddenFiles.length > 0) {
values = values.filter((v) => !hiddenFiles.includes(v))
}
} catch (e) {
console.error('Failed to read hidden files from localStorage', e)
}
if (values.length > 12) {
values = values.slice(0, 12)
}
setting.meshWidget.options.values = values
}
if (setting.meshWidget.options?.values) {
const rawValues = setting.meshWidget.options.values
if (!Array.isArray(rawValues)) {
return
}
let values = rawValues.filter((v): v is string => typeof v === 'string')
try {
const stored = localStorage.getItem('Comfy.Load3D.HiddenFiles')
const hiddenFiles = stored ? JSON.parse(stored) : []
if (hiddenFiles.length > 0) {
values = values.filter((v) => !hiddenFiles.includes(v))
}
} catch (e) {
console.error('Failed to read hidden files from localStorage', e)
}
if (values.length > 12) {
values = values.slice(0, 12)
}
setting.meshWidget.options.values = values
}
🤖 Prompt for AI Agents
In @src/extensions/core/load3d/Load3DConfiguration.ts around lines 43-61, The
code casts setting.meshWidget.options.values to string[] without verifying its
shape; add a type guard to ensure it's an array of strings before using it
(e.g., check Array.isArray(setting.meshWidget.options.values) and filter
elements with typeof v === 'string'), fallback to an empty array if the guard
fails, then continue using the validated string[] for the localStorage
filtering/slicing and reassign back to setting.meshWidget.options.values.

🧹 Nitpick | 🔵 Trivial

Consider using immutable operations for filtering and slicing.

The current implementation mutates the values variable through reassignment. While this works, using immutable patterns would be more aligned with functional programming best practices and coding guidelines.

🔎 Proposed refactor using immutability
 if (setting.meshWidget.options?.values) {
-  let values = setting.meshWidget.options.values as string[]
+  const rawValues = setting.meshWidget.options.values
+  if (!Array.isArray(rawValues)) {
+    return
+  }
+  
+  const validValues = rawValues.filter((v): v is string => typeof v === 'string')

   try {
     const stored = localStorage.getItem('Comfy.Load3D.HiddenFiles')
     const hiddenFiles = stored ? JSON.parse(stored) : []
-    if (hiddenFiles.length > 0) {
-      values = values.filter((v) => !hiddenFiles.includes(v))
-    }
+    const filteredValues = hiddenFiles.length > 0
+      ? validValues.filter((v) => !hiddenFiles.includes(v))
+      : validValues
+    
+    const limitedValues = filteredValues.length > 12
+      ? filteredValues.slice(0, 12)
+      : filteredValues
+    
+    setting.meshWidget.options.values = limitedValues
   } catch (e) {
     console.error('Failed to read hidden files from localStorage', e)
+    setting.meshWidget.options.values = validValues.slice(0, 12)
   }
-
-  if (values.length > 12) {
-    values = values.slice(0, 12)
-  }
-
-  setting.meshWidget.options.values = values
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (setting.meshWidget.options?.values) {
let values = setting.meshWidget.options.values as string[]
try {
const stored = localStorage.getItem('Comfy.Load3D.HiddenFiles')
const hiddenFiles = stored ? JSON.parse(stored) : []
if (hiddenFiles.length > 0) {
values = values.filter((v) => !hiddenFiles.includes(v))
}
} catch (e) {
console.error('Failed to read hidden files from localStorage', e)
}
if (values.length > 12) {
values = values.slice(0, 12)
}
setting.meshWidget.options.values = values
}
if (setting.meshWidget.options?.values) {
const rawValues = setting.meshWidget.options.values
if (!Array.isArray(rawValues)) {
return
}
const validValues = rawValues.filter(
(v): v is string => typeof v === 'string'
)
try {
const stored = localStorage.getItem('Comfy.Load3D.HiddenFiles')
const hiddenFiles = stored ? JSON.parse(stored) : []
const filteredValues = hiddenFiles.length > 0
? validValues.filter((v) => !hiddenFiles.includes(v))
: validValues
const limitedValues = filteredValues.length > 12
? filteredValues.slice(0, 12)
: filteredValues
setting.meshWidget.options.values = limitedValues
} catch (e) {
console.error('Failed to read hidden files from localStorage', e)
setting.meshWidget.options.values = validValues.slice(0, 12)
}
}
🤖 Prompt for AI Agents
In @src/extensions/core/load3d/Load3DConfiguration.ts around lines 43-61, The
code mutates the local variable values via reassignment; change to an immutable
flow: keep the original array (e.g., originalValues =
setting.meshWidget.options.values as string[]), then compute a new array by
applying filter based on parsed hiddenFiles and then applying slice(0, 12) as
needed, and finally assign that new array to setting.meshWidget.options.values;
reference the symbol setting.meshWidget.options.values and the localStorage key
'Comfy.Load3D.HiddenFiles' when locating the logic to refactor.


this.setupTargetSize(setting.width, setting.height)
this.setupDefaultProperties(setting.bgImagePath)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
:allow-upload="allowUpload"
:upload-folder="uploadFolder"
:is-asset-mode="isAssetMode"
:upload-subfolder="uploadSubfolder"
:default-layout-mode="defaultLayoutMode"
/>
<WidgetWithControl
Expand Down Expand Up @@ -58,9 +59,13 @@ const specDescriptor = computed<{
kind: AssetKind
allowUpload: boolean
folder: ResultItemType | undefined
subfolder?: string
}>(() => {
const isLoad3DModel =
props.nodeType === 'Load3D' && props.widget.name === 'model_file'
Comment on lines +64 to +65

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider extracting magic strings to constants.

The hardcoded 'Load3D' and 'model_file' strings work correctly but could become maintenance concerns if these identifiers change or are referenced elsewhere. Consider extracting them to named constants for better maintainability and searchability.

🔎 Suggested improvement
+const LOAD_3D_NODE_TYPE = 'Load3D'
+const MODEL_FILE_WIDGET_NAME = 'model_file'
+
 const specDescriptor = computed<{
   kind: AssetKind
   allowUpload: boolean
   folder: ResultItemType | undefined
   subfolder?: string
 }>(() => {
   const isLoad3DMesh =
-    props.nodeType === 'Load3D' && props.widget.name === 'model_file'
+    props.nodeType === LOAD_3D_NODE_TYPE && props.widget.name === MODEL_FILE_WIDGET_NAME
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const isLoad3DMesh =
props.nodeType === 'Load3D' && props.widget.name === 'model_file'
const LOAD_3D_NODE_TYPE = 'Load3D'
const MODEL_FILE_WIDGET_NAME = 'model_file'
const specDescriptor = computed<{
kind: AssetKind
allowUpload: boolean
folder: ResultItemType | undefined
subfolder?: string
}>(() => {
const isLoad3DMesh =
props.nodeType === LOAD_3D_NODE_TYPE &&
props.widget.name === MODEL_FILE_WIDGET_NAME
🤖 Prompt for AI Agents
In @src/renderer/extensions/vueNodes/widgets/components/WidgetSelect.vue around
lines 64-65, Extract the magic strings used in the isLoad3DMesh expression into
named constants (e.g., LOAD3D_NODE_TYPE and MODEL_FILE_WIDGET) and replace the
inline literals in the check (currently using props.nodeType === 'Load3D' and
props.widget.name === 'model_file') with those constants; define the constants
near the top of WidgetSelect.vue (or in a shared constants module if used
elsewhere) and update any other occurrences to use the new constant names to
improve maintainability and searchability.


const spec = comboSpec.value
if (!spec) {
if (!spec && !isLoad3DModel) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider extracting magic strings to constants.

The hardcoded 'Load3D' and 'model_file' strings work correctly but could become maintenance concerns if these identifiers change or are referenced elsewhere. Consider extracting them to named constants for better maintainability and searchability.

🔎 Suggested improvement
+const LOAD_3D_NODE_TYPE = 'Load3D'
+const MODEL_FILE_WIDGET_NAME = 'model_file'
+
 const specDescriptor = computed<{
   kind: AssetKind
   allowUpload: boolean
   folder: ResultItemType | undefined
   subfolder?: string
 }>(() => {
   const isLoad3DModel =
-    props.nodeType === 'Load3D' && props.widget.name === 'model_file'
+    props.nodeType === LOAD_3D_NODE_TYPE && props.widget.name === MODEL_FILE_WIDGET_NAME

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In src/renderer/extensions/vueNodes/widgets/components/WidgetSelect.vue around
lines 64 to 68, the literal strings 'Load3D' and 'model_file' are used directly;
extract these magic strings into clearly named constants (e.g.,
LOAD_3D_NODE_TYPE and MODEL_FILE_WIDGET_NAME) at the top of the module or in a
shared constants file, replace the inline literals with those constants, and
update any other occurrences in the file to use the constants for consistency
and easier maintenance.

return {
kind: 'unknown',
allowUpload: false,
Expand All @@ -74,7 +79,7 @@ const specDescriptor = computed<{
video_upload,
image_folder,
audio_upload
} = spec
} = spec || {}

let kind: AssetKind = 'unknown'
if (video_upload) {
Expand All @@ -83,18 +88,25 @@ const specDescriptor = computed<{
kind = 'image'
} else if (audio_upload) {
kind = 'audio'
} else if (isLoad3DModel) {
kind = 'model'
}
// TODO: add support for models (checkpoints, VAE, LoRAs, etc.) -- get widgetType from spec

const allowUpload =
image_upload === true ||
animated_image_upload === true ||
video_upload === true ||
audio_upload === true
audio_upload === true ||
isLoad3DModel

const subfolder = isLoad3DModel ? '3d' : undefined
const folder = isLoad3DModel ? 'input' : image_folder

return {
kind,
allowUpload,
folder: image_folder
folder,
subfolder
}
})

Expand All @@ -120,6 +132,7 @@ const allowUpload = computed(() => specDescriptor.value.allowUpload)
const uploadFolder = computed<ResultItemType>(() => {
return specDescriptor.value.folder ?? 'input'
})
const uploadSubfolder = computed(() => specDescriptor.value.subfolder)
const defaultLayoutMode = computed<LayoutMode>(() => {
return isAssetMode.value ? 'list' : 'grid'
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ interface Props {
assetKind?: AssetKind
allowUpload?: boolean
uploadFolder?: ResultItemType
uploadSubfolder?: string
isAssetMode?: boolean
defaultLayoutMode?: LayoutMode
}
Expand Down Expand Up @@ -82,10 +83,6 @@ const filterOptions = computed<FilterOption[]>(() => {

const selectedSet = ref<Set<SelectedKey>>(new Set())

/**
* Transforms a value using getOptionLabel if available.
* Falls back to the original value if getOptionLabel is not provided or throws an error.
*/
function getDisplayLabel(value: string): string {
const getOptionLabel = props.widget.options?.getOptionLabel
if (!getOptionLabel) return value
Expand Down Expand Up @@ -118,7 +115,6 @@ const outputItems = computed<DropdownItem[]>(() => {

const outputs = new Set<string>()

// Extract output images/videos from queue history
queueStore.historyTasks.forEach((task) => {
task.flatOutputs.forEach((output) => {
const isTargetType =
Expand All @@ -129,7 +125,6 @@ const outputItems = computed<DropdownItem[]>(() => {
const path = output.subfolder
? `${output.subfolder}/${output.filename}`
: output.filename
// Add [output] annotation so the preview component knows the type
const annotatedPath = `${path} [output]`
outputs.add(annotatedPath)
}
Expand Down Expand Up @@ -197,17 +192,17 @@ const uploadable = computed(() => {
})

const acceptTypes = computed(() => {
// Be permissive with accept types because backend uses libraries
// that can handle a wide range of formats
switch (props.assetKind) {
case 'image':
return 'image/*'
case 'video':
return 'video/*'
case 'audio':
return 'audio/*'
case 'model':
return '.obj,.stl,.ply,.spz'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for 3D model file extension validation or processing in backend
rg -n '\.(obj|stl|ply|spz|fbx|dae|gltf|glb)' --type-list | head -20

# Search for upload endpoint implementations
rg -n 'upload' --type-list | grep -E '(route|endpoint|def.*upload)' | head -20

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 849


🏁 Script executed:

# Search for Python files related to uploads
fd -e py -type f -path '*upload*' 

# Search for file extension validation patterns
rg -n 'accept|extension|format' -t py -A3 -B3 | head -50

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 240


🏁 Script executed:

# Look for backend model configuration or constants
rg -n 'obj|stl|ply|spz' -t py -A2 -B2

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 1021


🏁 Script executed:

# Check the current file to understand the context better
find . -name 'WidgetSelectDropdown.vue' -exec cat -n {} \;

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 13058


🏁 Script executed:

#!/bin/bash
# Search for upload endpoint handlers in backend
find . -type f -name "*.py" | xargs grep -l "upload" | head -10

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 102


🏁 Script executed:

#!/bin/bash
# Search for .spz format references
rg -n "spz" -i

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 50384


🏁 Script executed:

#!/bin/bash
# Search for useNodeImageUpload to understand where this was copied from
fd -e ts -e vue | xargs grep -l "useNodeImageUpload" 2>/dev/null

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 266


🏁 Script executed:

#!/bin/bash
# Search for model format constants or configurations
rg -n "obj|stl|ply" -A2 -B2

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 50386


🏁 Script executed:

#!/bin/bash
# Check if there are any other references to acceptTypes or model uploads
rg -n "acceptTypes|assetKind.*model" -A3 -B3

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 1634


Update the accept types for 3D model uploads to include all supported formats.

The widget currently restricts 3D model uploads to .obj,.stl,.ply,.spz, but src/extensions/core/load3d.ts reveals the backend supports a broader set of formats: .gltf,.glb,.obj,.fbx,.stl,.ply,.spz,.splat,.ksplat. The comment at lines 201-202 indicates the backend uses libraries "that can handle a wide range of formats," yet the frontend restricts them unnecessarily.

Update line 211 to include at least the commonly used formats:

Suggested update
case 'model':
  return '.obj,.stl,.ply,.spz,.fbx,.gltf,.glb'

Or include the full set for consistency with backend capabilities:

case 'model':
  return '.obj,.stl,.ply,.spz,.fbx,.gltf,.glb,.splat,.ksplat'
🤖 Prompt for AI Agents
In src/renderer/extensions/vueNodes/widgets/components/WidgetSelectDropdown.vue
around lines 210-211, the accept list for 'model' currently limits uploads to
.obj,.stl,.ply,.spz but the backend supports more formats; update the returned
string to include the additional supported types (at minimum .fbx,.gltf,.glb, or
include the full set .splat and .ksplat) so the frontend accept attribute
matches backend capabilities; modify the case 'model' return value accordingly.

Comment on lines +211 to +212

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Update accept types to include all supported 3D formats.

The current accept types for 'mesh' only include .obj,.stl,.ply,.spz, but src/extensions/core/load3d.ts (lines 253-254) shows the backend supports additional formats: .gltf,.glb,.fbx,.splat,.ksplat. Restricting the frontend unnecessarily prevents users from uploading valid formats.

🔎 Proposed fix
     case 'mesh':
-      return '.obj,.stl,.ply,.spz'
+      return '.obj,.stl,.ply,.spz,.fbx,.gltf,.glb,.splat,.ksplat'
🤖 Prompt for AI Agents
In @src/renderer/extensions/vueNodes/widgets/components/WidgetSelectDropdown.vue
around lines 211-212, The accept list for the 'mesh' option is missing several
supported 3D formats; update the returned string in the WidgetSelectDropdown.vue
branch that handles case 'mesh' (the return '.obj,.stl,.ply,.spz' expression) to
include the additional extensions .gltf, .glb, .fbx, .splat, and .ksplat so the
frontend matches the backend supported formats.

default:
return undefined // model or unknown
return undefined
}
})

Expand Down Expand Up @@ -247,15 +242,16 @@ function updateSelectedItems(selectedItems: Set<SelectedKey>) {
modelValue.value = name
}

// Upload file function (copied from useNodeImageUpload.ts)
const uploadFile = async (
file: File,
isPasted: boolean = false,
formFields: Partial<{ type: ResultItemType }> = {}
formFields: Partial<{ type: ResultItemType; subfolder: string }> = {}
) => {
const body = new FormData()
body.append('image', file)
if (isPasted) body.append('subfolder', 'pasted')
else if (formFields.subfolder) body.append('subfolder', formFields.subfolder)

if (formFields.type) body.append('type', formFields.type)

const resp = await api.fetchApi('/upload/image', {
Expand All @@ -270,7 +266,6 @@ const uploadFile = async (

const data = await resp.json()

// Update AssetsStore when uploading to input folder
if (formFields.type === 'input' || (!formFields.type && !isPasted)) {
const assetsStore = useAssetsStore()
await assetsStore.updateInputs()
Expand All @@ -279,11 +274,11 @@ const uploadFile = async (
return data.subfolder ? `${data.subfolder}/${data.name}` : data.name
}

// Handle multiple file uploads
const uploadFiles = async (files: File[]): Promise<string[]> => {
const folder = props.uploadFolder ?? 'input'
const subfolder = props.uploadSubfolder
const uploadPromises = files.map((file) =>
uploadFile(file, false, { type: folder })
uploadFile(file, false, { type: folder, subfolder })
)
const results = await Promise.all(uploadPromises)
return results.filter((path): path is string => path !== null)
Expand All @@ -293,29 +288,31 @@ async function handleFilesUpdate(files: File[]) {
if (!files || files.length === 0) return

try {
// 1. Upload files to server
const uploadedPaths = await uploadFiles(files)

if (uploadedPaths.length === 0) {
toastStore.addAlert('File upload failed')
return
}

// 2. Update widget options to include new files
// This simulates what addToComboValues does but for SimplifiedWidget
if (props.widget.options?.values) {
uploadedPaths.forEach((path) => {
const values = props.widget.options!.values as string[]
if (!values.includes(path)) {
values.push(path)
const values = props.widget.options.values as string[]

uploadedPaths.reverse().forEach((path) => {
const existingIndex = values.indexOf(path)
if (existingIndex > -1) {
values.splice(existingIndex, 1)
}
values.unshift(path)
})
Comment on lines +317 to 324

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Logic bug: modelValue and dropdown order are inconsistent for multi-file uploads.

The reverse() + unshift() pattern results in the first uploaded file at values[0], but modelValue is set to uploadedPaths[0] which, after the in-place reverse, is the last uploaded file. This creates a confusing UX where the selected value doesn't match the top dropdown item.

Trace for uploadedPaths = ['a.obj', 'b.obj', 'c.obj']:

  • After reverse(): ['c.obj', 'b.obj', 'a.obj']
  • After forEach with unshift: values = ['a.obj', 'b.obj', 'c.obj', ...existing]
  • modelValue = uploadedPaths[0] = 'c.obj'

Result: 'a.obj' is at top of dropdown, but 'c.obj' is selected.

🔎 Proposed fix to align modelValue with values[0]
     if (props.widget.options?.values) {
       const values = props.widget.options.values as string[]

-      // Reverse uploadedPaths so the very last uploaded is at absolute top if multiple
-      uploadedPaths.reverse().forEach((path) => {
+      // Add uploaded paths to top, maintaining upload order (first uploaded at top)
+      ;[...uploadedPaths].reverse().forEach((path) => {
         // Remove existing duplicates to move them to top
         const existingIndex = values.indexOf(path)
         if (existingIndex > -1) {
           values.splice(existingIndex, 1)
         }
         values.unshift(path)
       })

       // Enforce limit of 12
       if (values.length > 12) {
         values.splice(12)
       }
     }
     // 3. Update widget value to the first uploaded file
-    modelValue.value = uploadedPaths[0]
+    modelValue.value = uploadedPaths[0] // Now correctly matches values[0]

By spreading into a new array before reversing, the original uploadedPaths order is preserved for modelValue.

Also applies to: 331-331

🤖 Prompt for AI Agents
In src/renderer/extensions/vueNodes/widgets/components/WidgetSelectDropdown.vue
around lines 316-323 (and similarly at line 331), the code uses
uploadedPaths.reverse() which mutates the original array and causes modelValue
to reference the wrong element; instead, create a non-mutating reversed copy
(e.g. const toInsert = [...uploadedPaths].reverse()), iterate to remove
duplicates and unshift from toInsert so values is built correctly, then set the
modelValue to values[0] (or derive modelValue from the non-mutated ordering) so
the selected item matches the top dropdown entry; apply the same non-mutating
approach at the other occurrence on line 331.


if (values.length > 12) {
values.splice(12)
}
Comment on lines +314 to +329

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add type guard and simplify array manipulation.

The code has two concerns:

  1. Type safety: Casts props.widget.options.values to string[] without verifying it's actually an array of strings (similar issue as in Load3DConfiguration.ts).
  2. Efficiency: The reverse().forEach() with splice and unshift operations results in O(n²) complexity.
🔎 Proposed fix with type guard and cleaner logic
     if (props.widget.options?.values) {
-      const values = props.widget.options.values as string[]
+      const rawValues = props.widget.options.values
+      if (!Array.isArray(rawValues)) {
+        return
+      }
+      
+      const values = rawValues.filter((v): v is string => typeof v === 'string')
+      const uploadedSet = new Set(uploadedPaths)
+      const filtered = values.filter((v) => !uploadedSet.has(v))
+      const updated = [...uploadedPaths, ...filtered].slice(0, 12)

-      uploadedPaths.reverse().forEach((path) => {
-        const existingIndex = values.indexOf(path)
-        if (existingIndex > -1) {
-          values.splice(existingIndex, 1)
-        }
-        values.unshift(path)
-      })
-
-      if (values.length > 12) {
-        values.splice(12)
-      }
+      props.widget.options.values = updated
     }

This approach:

  • Adds proper type validation
  • Eliminates O(n²) operations
  • Is more readable and functional
  • Maintains the same deduplication and ordering behavior
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const values = props.widget.options.values as string[]
uploadedPaths.reverse().forEach((path) => {
const existingIndex = values.indexOf(path)
if (existingIndex > -1) {
values.splice(existingIndex, 1)
}
values.unshift(path)
})
if (values.length > 12) {
values.splice(12)
}
const rawValues = props.widget.options.values
if (!Array.isArray(rawValues)) {
return
}
const values = rawValues.filter((v): v is string => typeof v === 'string')
const uploadedSet = new Set(uploadedPaths)
const filtered = values.filter((v) => !uploadedSet.has(v))
const updated = [...uploadedPaths, ...filtered].slice(0, 12)
props.widget.options.values = updated
🤖 Prompt for AI Agents
In src/renderer/extensions/vueNodes/widgets/components/WidgetSelectDropdown.vue
around lines 299-311, add a type guard to ensure props.widget.options.values is
actually an array of strings (fall back to an empty array if not), then replace
the reverse+splice+unshift O(n²) loop with a linear approach: build a new array
by first taking uploadedPaths (filtered to strings and preserving their original
order), then append the existing values that are not in the uploadedPaths set to
avoid duplicates, finally truncate the resulting array to 12 entries and assign
it back to props.widget.options.values; this preserves behavior, ensures type
safety, and reduces complexity to O(n).

}

// 3. Update widget value to the first uploaded file
modelValue.value = uploadedPaths[0]
Comment on lines +314 to 332

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: Fix type safety and array mutation bugs in upload handling.

This code has multiple critical issues identified in previous reviews:

  1. Type safety: Line 314 casts to string[] without validation
  2. Logic bug: Line 317 mutates uploadedPaths in place with reverse(), causing modelValue (line 332) to reference the wrong element (last uploaded instead of first)
  3. Performance: O(n²) complexity with nested operations

For uploadedPaths = ['a.obj', 'b.obj', 'c.obj']:

  • After in-place reverse(): ['c.obj', 'b.obj', 'a.obj']
  • After forEach: values = ['a.obj', 'b.obj', 'c.obj', ...]
  • modelValue = uploadedPaths[0] = 'c.obj' ❌ (should be 'a.obj')
🔎 Proposed fix
     if (props.widget.options?.values) {
-      const values = props.widget.options.values as string[]
+      const rawValues = props.widget.options.values
+      if (!Array.isArray(rawValues)) {
+        return
+      }
+      
+      const values = rawValues.filter((v): v is string => typeof v === 'string')
+      const uploadedSet = new Set(uploadedPaths)
+      const filtered = values.filter((v) => !uploadedSet.has(v))
+      const updated = [...uploadedPaths, ...filtered].slice(0, 12)

-      // Reverse uploadedPaths so the very last uploaded is at absolute top if multiple
-      uploadedPaths.reverse().forEach((path) => {
-        // Remove existing duplicates to move them to top
-        const existingIndex = values.indexOf(path)
-        if (existingIndex > -1) {
-          values.splice(existingIndex, 1)
-        }
-        values.unshift(path)
-      })
-
-      // Enforce limit of 12
-      if (values.length > 12) {
-        values.splice(12)
-      }
+      props.widget.options.values = updated
     }
-    // 3. Update widget value to the first uploaded file
     modelValue.value = uploadedPaths[0]
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const values = props.widget.options.values as string[]
// Reverse uploadedPaths so the very last uploaded is at absolute top if multiple
uploadedPaths.reverse().forEach((path) => {
// Remove existing duplicates to move them to top
const existingIndex = values.indexOf(path)
if (existingIndex > -1) {
values.splice(existingIndex, 1)
}
values.unshift(path)
})
}
// Enforce limit of 12
if (values.length > 12) {
values.splice(12)
}
}
// 3. Update widget value to the first uploaded file
modelValue.value = uploadedPaths[0]
if (props.widget.options?.values) {
const rawValues = props.widget.options.values
if (!Array.isArray(rawValues)) {
return
}
const values = rawValues.filter((v): v is string => typeof v === 'string')
const uploadedSet = new Set(uploadedPaths)
const filtered = values.filter((v) => !uploadedSet.has(v))
const updated = [...uploadedPaths, ...filtered].slice(0, 12)
props.widget.options.values = updated
}
modelValue.value = uploadedPaths[0]
🤖 Prompt for AI Agents
In @src/renderer/extensions/vueNodes/widgets/components/WidgetSelectDropdown.vue
around lines 314-332, The upload handling mutates uploadedPaths with reverse(),
casts props.widget.options.values to string[] without checking, and does O(n²)
splices; fix by: validate props.widget.options.values and uploadedPaths using
Array.isArray and filter to strings before use (refer to
props.widget.options.values and uploadedPaths), work on non-mutating copies
(e.g., const uploads = [...uploadedPathsFiltered]) and reverse the copy (not
uploadedPaths) so original order is preserved, dedupe using a Set to avoid
repeated indexOf/splice (avoid O(n²)) while building the new values array,
enforce the 12-item cap on the new array, and finally set modelValue.value to
the true first uploaded item from the validated uploads array (e.g., uploads[0])
so it references the first uploaded file.


// 4. Trigger callback to notify underlying LiteGraph widget
if (props.widget.callback) {
props.widget.callback(uploadedPaths[0])
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,15 +65,13 @@ function handleVideoLoad(event: Event) {
layout === 'list',
'flex-row text-left hover:bg-component-node-widget-background-hovered rounded-lg':
layout === 'list-small',
// selection
'ring-2 ring-component-node-widget-background-highlighted':
layout === 'list' && selected
}
)
"
@click="handleClick"
>
<!-- Image -->
<div
v-if="layout !== 'list-small'"
:class="
Expand All @@ -85,14 +83,12 @@ function handleVideoLoad(event: Event) {
'min-w-16 max-w-16 rounded-l-lg': layout === 'list',
'rounded-sm group-hover/item:scale-108 group-active/item:scale-95':
layout === 'grid',
// selection
'ring-2 ring-component-node-widget-background-highlighted':
layout === 'grid' && selected
}
)
"
>
<!-- Selected Icon -->
<div
v-if="selected"
class="absolute top-1 left-1 size-4 rounded-full border-1 border-base-foreground bg-primary-background"
Expand All @@ -118,10 +114,11 @@ function handleVideoLoad(event: Event) {
/>
<div
v-else
class="size-full bg-gradient-to-tr from-blue-400 via-teal-500 to-green-400"
/>
class="size-full flex items-center justify-center bg-slate-50"
>
<i class="pi pi-box text-slate-400" style="font-size: 2.5rem" />
</div>
Comment on lines 119 to +124

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Replace hardcoded colors with semantic theme values.

The placeholder uses hardcoded Tailwind colors (bg-slate-50, text-slate-400) and an inline style for font size. Per coding guidelines, use semantic theme values from style.css instead of hardcoded colors, and prefer Tailwind utility classes over inline styles.

🔎 Proposed refactor using semantic values
       <div
         v-else
-        class="size-full flex items-center justify-center bg-slate-50"
+        class="size-full flex items-center justify-center bg-component-node-widget-background"
       >
-        <i class="pi pi-box text-slate-400" style="font-size: 2.5rem" />
+        <i class="pi pi-box text-[2.5rem] text-muted-foreground" />
       </div>

Note: Verify that bg-component-node-widget-background and text-muted-foreground are the appropriate semantic tokens in your theme. Adjust based on the actual theme definitions in style.css.

As per coding guidelines: "Do not use the dark: Tailwind variant; use semantic values from the style.css theme instead (e.g., bg-node-component-surface)."

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In
src/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdownMenuItem.vue
lines 115-120, replace hardcoded Tailwind color classes and the inline font-size
with semantic theme values and Tailwind utilities: remove bg-slate-50 and
text-slate-400 and use the appropriate semantic classes from style.css (e.g.,
bg-component-node-widget-background and text-muted-foreground), and replace the
inline style="font-size: 2.5rem" with a Tailwind font-size utility (e.g.,
text-[2.5rem] or the nearest predefined text-3xl/text-4xl) so the component uses
theme tokens and utility classes instead of hardcoded colors and inline styles.

</div>
<!-- Name -->
<div
:class="
cn('flex gap-1', {
Expand All @@ -138,14 +135,12 @@ function handleVideoLoad(event: Event) {
cn(
'block text-xs line-clamp-2 break-words overflow-hidden',
'transition-colors duration-150',
// selection
!!selected && 'text-base-foreground'
)
"
>
{{ label ?? name }}
</span>
<!-- Meta Data -->
<span class="text-secondary block text-xs">{{
metadata || actualDimensions
}}</span>
Expand Down