Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
16 changes: 16 additions & 0 deletions src/composables/useLoad3d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -511,6 +511,22 @@ export const useLoad3d = (nodeOrRef: MaybeRef<LGraphNode | null>) => {
hasSkeleton.value = load3d?.hasSkeleton() ?? false
// Reset skeleton visibility when loading new model
modelConfig.value.showSkeleton = false

if (load3d) {
const node = nodeRef.value

const modelWidget = node?.widgets?.find(
(w) => w.name === 'model_file' || w.name === 'image'
)
const value = modelWidget?.value
if (typeof value === 'string') {
void Load3dUtils.generateThumbnailIfNeeded(
load3d,
value,
isPreview.value ? 'output' : 'input'
)
}
}
},
skeletonVisibilityChange: (value: boolean) => {
modelConfig.value.showSkeleton = value
Expand Down
54 changes: 54 additions & 0 deletions src/extensions/core/load3d/Load3d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -754,6 +754,60 @@ class Load3d {
this.forceRender()
}

public async captureThumbnail(
width: number = 256,
height: number = 256
): Promise<string> {
if (!this.modelManager.currentModel) {
throw new Error('No model loaded for thumbnail capture')
}

const savedState = this.cameraManager.getCameraState()
const savedCameraType = this.cameraManager.getCurrentCameraType()
const savedGridVisible = this.sceneManager.gridHelper.visible

try {
this.sceneManager.gridHelper.visible = false

if (savedCameraType !== 'perspective') {
this.cameraManager.toggleCamera('perspective')
}

const box = new THREE.Box3().setFromObject(this.modelManager.currentModel)
const size = box.getSize(new THREE.Vector3())
const center = box.getCenter(new THREE.Vector3())

const maxDim = Math.max(size.x, size.y, size.z)
const distance = maxDim * 1.5

const cameraPosition = new THREE.Vector3(
center.x - distance * 0.8,
center.y + distance * 0.4,
center.z + distance * 0.3
)

this.cameraManager.perspectiveCamera.position.copy(cameraPosition)
this.cameraManager.perspectiveCamera.lookAt(center)
this.cameraManager.perspectiveCamera.updateProjectionMatrix()

if (this.controlsManager.controls) {
this.controlsManager.controls.target.copy(center)
this.controlsManager.controls.update()
}

const result = await this.sceneManager.captureScene(width, height)
return result.scene
} finally {
this.sceneManager.gridHelper.visible = savedGridVisible

if (savedCameraType !== 'perspective') {
this.cameraManager.toggleCamera(savedCameraType)
}
this.cameraManager.setCameraState(savedState)
this.controlsManager.controls?.update()
Comment on lines +772 to +807
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Keep controls/view helper in sync when switching cameras.

cameraManager.toggleCamera(...) bypasses Load3d.toggleCamera, so controls and view helper may remain bound to the previous camera type after thumbnail capture. This can leave interactions broken if the user was in orthographic mode.

🐛 Proposed fix (minimal side effects)
-      if (savedCameraType !== 'perspective') {
-        this.cameraManager.toggleCamera('perspective')
-      }
+      if (savedCameraType !== 'perspective') {
+        this.cameraManager.toggleCamera('perspective')
+        this.controlsManager.updateCamera(this.cameraManager.activeCamera)
+        this.viewHelperManager.recreateViewHelper()
+      }
@@
-      if (savedCameraType !== 'perspective') {
-        this.cameraManager.toggleCamera(savedCameraType)
-      }
+      if (savedCameraType !== 'perspective') {
+        this.cameraManager.toggleCamera(savedCameraType)
+        this.controlsManager.updateCamera(this.cameraManager.activeCamera)
+        this.viewHelperManager.recreateViewHelper()
+      }
🤖 Prompt for AI Agents
In `@src/extensions/core/load3d/Load3d.ts` around lines 772 - 807, Replace direct
calls to cameraManager.toggleCamera with the class-level toggle that keeps
controls and view helper in sync: call this.toggleCamera('perspective') before
capturing and this.toggleCamera(savedCameraType) in the finally block instead of
this.cameraManager.toggleCamera(...). This ensures controlsManager.controls and
any view helper bindings are updated when switching cameras during the thumbnail
flow (references: Load3d.toggleCamera, cameraManager.toggleCamera,
controlsManager.controls).

}
}

public remove(): void {
if (this.contextMenuAbortController) {
this.contextMenuAbortController.abort()
Expand Down
65 changes: 65 additions & 0 deletions src/extensions/core/load3d/Load3dUtils.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,34 @@
import type Load3d from '@/extensions/core/load3d/Load3d'
import { t } from '@/i18n'
import { useToastStore } from '@/platform/updates/common/toastStore'
import { api } from '@/scripts/api'
import { app } from '@/scripts/app'

class Load3dUtils {
static async generateThumbnailIfNeeded(
load3d: Load3d,
modelPath: string,
folderType: 'input' | 'output'
): Promise<void> {
const [subfolder, filename] = this.splitFilePath(modelPath)
const thumbnailFilename = this.getThumbnailFilename(filename)

const exists = await this.fileExists(
subfolder,
thumbnailFilename,
folderType
)
if (exists) return

const imageData = await load3d.captureThumbnail(256, 256)
await this.uploadThumbnail(
imageData,
subfolder,
thumbnailFilename,
folderType
)
}
Comment on lines +8 to +30
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Prevent unhandled rejections from fire‑and‑forget thumbnail generation.

Callers invoke this with void, so any failure will surface as an unhandled promise rejection. Wrap the flow in a try/catch and log once. As per coding guidelines, ensure proper error handling.

🐛 Proposed fix
   static async generateThumbnailIfNeeded(
     load3d: Load3d,
     modelPath: string,
     folderType: 'input' | 'output'
   ): Promise<void> {
-    const [subfolder, filename] = this.splitFilePath(modelPath)
-    const thumbnailFilename = this.getThumbnailFilename(filename)
-
-    const exists = await this.fileExists(
-      subfolder,
-      thumbnailFilename,
-      folderType
-    )
-    if (exists) return
-
-    const imageData = await load3d.captureThumbnail(256, 256)
-    await this.uploadThumbnail(
-      imageData,
-      subfolder,
-      thumbnailFilename,
-      folderType
-    )
+    try {
+      const [subfolder, filename] = this.splitFilePath(modelPath)
+      const thumbnailFilename = this.getThumbnailFilename(filename)
+
+      const exists = await this.fileExists(
+        subfolder,
+        thumbnailFilename,
+        folderType
+      )
+      if (exists) return
+
+      const imageData = await load3d.captureThumbnail(256, 256)
+      await this.uploadThumbnail(
+        imageData,
+        subfolder,
+        thumbnailFilename,
+        folderType
+      )
+    } catch (error) {
+      console.warn('[Load3D] generateThumbnailIfNeeded failed:', error)
+    }
   }
📝 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
static async generateThumbnailIfNeeded(
load3d: Load3d,
modelPath: string,
folderType: 'input' | 'output'
): Promise<void> {
const [subfolder, filename] = this.splitFilePath(modelPath)
const thumbnailFilename = this.getThumbnailFilename(filename)
const exists = await this.fileExists(
subfolder,
thumbnailFilename,
folderType
)
if (exists) return
const imageData = await load3d.captureThumbnail(256, 256)
await this.uploadThumbnail(
imageData,
subfolder,
thumbnailFilename,
folderType
)
}
static async generateThumbnailIfNeeded(
load3d: Load3d,
modelPath: string,
folderType: 'input' | 'output'
): Promise<void> {
try {
const [subfolder, filename] = this.splitFilePath(modelPath)
const thumbnailFilename = this.getThumbnailFilename(filename)
const exists = await this.fileExists(
subfolder,
thumbnailFilename,
folderType
)
if (exists) return
const imageData = await load3d.captureThumbnail(256, 256)
await this.uploadThumbnail(
imageData,
subfolder,
thumbnailFilename,
folderType
)
} catch (error) {
console.warn('[Load3D] generateThumbnailIfNeeded failed:', error)
}
}
🤖 Prompt for AI Agents
In `@src/extensions/core/load3d/Load3dUtils.ts` around lines 8 - 30, The
generateThumbnailIfNeeded flow can throw unhandled rejections because callers
use void; wrap the entire body of Load3dUtils.generateThumbnailIfNeeded in a
try/catch that logs a single error on failure (use processLogger or existing
logger), preserving context (include modelPath and folderType) and avoid
rethrowing so fire-and-forget callers don't produce unhandled promise
rejections; keep the existing logic (splitFilePath, getThumbnailFilename,
fileExists, captureThumbnail, uploadThumbnail) inside the try block and call
logger.error once in the catch with the caught error details.


static async uploadTempImage(
imageData: string,
prefix: string,
Expand Down Expand Up @@ -122,6 +147,46 @@ class Load3dUtils {

await Promise.all(uploadPromises)
}

static getThumbnailFilename(modelFilename: string): string {
return `${modelFilename}.png`
}

static async fileExists(
subfolder: string,
filename: string,
type: string = 'input'
): Promise<boolean> {
try {
const url = api.apiURL(this.getResourceURL(subfolder, filename, type))
const response = await fetch(url, { method: 'HEAD' })
return response.ok
} catch {
return false
}
}

static async uploadThumbnail(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we re-use the helpers that already exist -- e.g., the one that is 100 lines above this

static async uploadFile(file: File, subfolder: string) {

Or this


Not really for this PR, but after looking, we seem to have a lot of duplication across the codebase for the upload endpoint usage:

  • src/composables/node/useNodeImageUpload.ts:20-49 (most generic implementation)
  • src/composables/maskeditor/useMaskEditorSaver.ts:266-299
  • src/components/common/BackgroundImageUpload.vue:55-74
  • src/extensions/core/webcamCapture.ts
  • src/extensions/core/uploadAudio.ts:28-73
  • src/services/audioService.ts
  • src/renderer/extensions/vueNodes/widgets/components/WidgetSelectDropdown.vue

Copy link
Contributor

Choose a reason for hiding this comment

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

@coderabbitai make an Issue to centralize all the upload helpers listed above into a singlar shared util or service that these modules then import from. Allows easier refactoring later whenever we adjust the upload logic or parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

🧩 Analysis chain

@christian-byrne, I'll create an issue to track this refactoring work.


🏁 Script executed:

#!/bin/bash
# Create GitHub issue to centralize upload helper functions

gh issue create \
  --title "Centralize upload helper functions into shared utility" \
  --body "## Context

Multiple upload helper implementations are duplicated across the codebase, making it difficult to maintain and refactor upload logic consistently.

## Locations with duplicated upload functionality

- \`src/composables/node/useNodeImageUpload.ts:20-49\` (most generic implementation)
- \`src/composables/maskeditor/useMaskEditorSaver.ts:266-299\`
- \`src/components/common/BackgroundImageUpload.vue:55-74\`
- \`src/extensions/core/webcamCapture.ts\`
- \`src/extensions/core/uploadAudio.ts:28-73\`
- \`src/services/audioService.ts\`
- \`src/renderer/extensions/vueNodes/widgets/components/WidgetSelectDropdown.vue\`

## Proposal

Centralize these implementations into a single shared utility or service that all modules can import from. This will:
- Reduce code duplication
- Make future changes to upload logic easier to maintain
- Ensure consistent behavior across all upload operations
- Simplify testing and error handling

## References

- Identified in PR `#8129`: https://github.com/Comfy-Org/ComfyUI_frontend/pull/8129
- Comment: https://github.com/Comfy-Org/ComfyUI_frontend/pull/8129#discussion_r2701437891
- Requested by: `@christian-byrne`" \
  --assignee christian-byrne

Length of output: 57


@christian-byrne, I've created issue #8130 to track the centralization of upload helper functions. The issue includes all the file locations you identified, the benefits of consolidating them, and references back to this PR and comment.


🧬 Code Graph Analysis Results

File: src/scripts/api.ts (lines 1304-1304)

export const api = new ComfyApi()

File: src/scripts/app.ts (lines 1746-1746)

export const app = new ComfyApp()

File: src/platform/updates/common/toastStore.ts (lines 8-39)

export const useToastStore = defineStore('toast', () => {
  const messagesToAdd = ref<ToastMessageOptions[]>([])
  const messagesToRemove = ref<ToastMessageOptions[]>([])
  const removeAllRequested = ref(false)

  function add(message: ToastMessageOptions) {
    messagesToAdd.value = [...messagesToAdd.value, message]
  }

  function remove(message: ToastMessageOptions) {
    messagesToRemove.value = [...messagesToRemove.value, message]
  }

  function removeAll() {
    removeAllRequested.value = true
  }

  function addAlert(message: string) {
    add({ severity: 'warn', summary: 'Alert', detail: message })
  }

  return {
    messagesToAdd,
    messagesToRemove,
    removeAllRequested,

    add,
    remove,
    removeAll,
    addAlert
  }
})

File: src/stores/queueStore.ts (lines 83-85)

get url(): string {
    return api.apiURL('/view?' + this.urlParams)
  }

🧠 Learnings used
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7746
File: src/platform/assets/composables/useUploadModelWizard.ts:207-219
Timestamp: 2026-01-06T19:17:12.355Z
Learning: In `src/platform/assets/composables/useUploadModelWizard.ts`, the `uploadModel()` function should have a concurrency guard at the beginning that checks if `isUploading.value` is already true and returns false early to prevent race conditions from multiple rapid invocations.

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-16T21:33:34.380Z
Learning: Applies to src/**/*.{ts,tsx,js,jsx,vue} : Ask if there is a simpler way to introduce functionality; choose the simpler course when available

imageData: string,
subfolder: string,
filename: string,
type: string = 'input'
): Promise<boolean> {
const blob = await fetch(imageData).then((r) => r.blob())
const file = new File([blob], filename, { type: 'image/png' })

const body = new FormData()
body.append('image', file)
body.append('subfolder', subfolder)
body.append('type', type)

const resp = await api.fetchApi('/upload/image', {
method: 'POST',
body
})

return resp.status === 200
}
}

export default Load3dUtils
12 changes: 12 additions & 0 deletions src/extensions/core/saveMesh.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import Load3D from '@/components/load3d/Load3D.vue'
import { useLoad3d } from '@/composables/useLoad3d'
import { createExportMenuItems } from '@/extensions/core/load3d/exportMenuHelper'
import Load3DConfiguration from '@/extensions/core/load3d/Load3DConfiguration'
import Load3dUtils from '@/extensions/core/load3d/Load3dUtils'
import type { LGraphNode } from '@/lib/litegraph/src/LGraphNode'
import type { IContextMenuValue } from '@/lib/litegraph/src/interfaces'
import type { NodeOutputWith, ResultItem } from '@/schemas/apiSchema'
Expand Down Expand Up @@ -94,6 +95,17 @@ useExtensionService().registerExtension({
const config = new Load3DConfiguration(load3d, node.properties)

const loadFolder = fileInfo.type as 'input' | 'output'

const onModelLoaded = () => {
load3d.removeEventListener('modelLoadingEnd', onModelLoaded)
void Load3dUtils.generateThumbnailIfNeeded(
load3d,
filePath,
loadFolder
)
}
load3d.addEventListener('modelLoadingEnd', onModelLoaded)

config.configureForSaveMesh(loadFolder, filePath)
}
})
Expand Down
29 changes: 26 additions & 3 deletions src/platform/assets/components/Media3DTop.vue
Original file line number Diff line number Diff line change
@@ -1,12 +1,35 @@
<template>
<div class="relative size-full overflow-hidden rounded">
<img
v-if="!thumbnailError"
:src="thumbnailSrc"
:alt="asset?.name"
class="size-full object-contain transition-transform duration-300 group-hover:scale-105 group-data-[selected=true]:scale-105"
@error="thumbnailError = true"
/>
<div
v-else
class="flex size-full flex-col items-center justify-center gap-2 bg-modal-card-placeholder-background transition-transform duration-300 group-hover:scale-105 group-data-[selected=true]:scale-105"
>
<i class="icon-[lucide--box] text-3xl text-muted-foreground" />
<span class="text-sm text-base-foreground">{{
$t('assetBrowser.media.threeDModelPlaceholder')
}}</span>
<span class="text-sm text-base-foreground">
{{ $t('assetBrowser.media.threeDModelPlaceholder') }}
</span>
</div>
</div>
</template>

<script setup lang="ts">
import { computed, ref } from 'vue'

import type { AssetMeta } from '../schemas/mediaAssetSchema'

const { asset } = defineProps<{ asset: AssetMeta }>()

const thumbnailError = ref(false)

const thumbnailSrc = computed(() => {
if (!asset?.src) return ''
return asset.src.replace(/([?&]filename=)([^&]*)/, '$1$2.png')
})
</script>