Consolidate image upload implementations into shared service #8186
+441
−102
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dosubot
bot
added
the
size:L
🎭 Playwright Tests: ✅ PassedResults: 507 passed, 0 failed, 0 flaky, 8 skipped (Total: 515) 📊 Browser Reports
|
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 01/30/2026, 07:01:10 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
Contributor
📝 WalkthroughWalkthroughAdds a centralized upload service (uploadMedia/uploadMediaBatch), refactors multiple upload sites to use it (Load3d utils and widget uploads), adds tests for the new service, minor template formatting, and guards backgroundImage assignment in useLoad3d.ts when upload path is falsy. Changes
Sequence Diagram(s)sequenceDiagram
participant Component as Component (Widget / Load3d)
participant UploadSvc as UploadService
participant API as api.fetchApi
participant Store as AssetsStore
participant UI as UI (Toast)
Component->>UploadSvc: uploadMedia(input, config) / uploadMediaBatch(inputs, config)
UploadSvc->>UploadSvc: convert input to File (File/Blob/dataURL)
UploadSvc->>UploadSvc: validate file size (maxSizeMB)
alt size OK
UploadSvc->>API: POST FormData (image, subfolder, type, original_ref)
API-->>UploadSvc: HTTP response (success or error)
alt success
UploadSvc-->>Component: { success: true, path, name }
Component->>Store: update assets (if subfolder matches)
Component->>UI: optional success UI / set value
else api error
UploadSvc-->>Component: { success: false, error, response? }
Component->>UI: show toast(error or generic)
end
else size exceeds
UploadSvc-->>Component: { success: false, error: "exceeds maximum" }
Component->>UI: show toast(fileTooLarge)
end
Suggested reviewers
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 |
coderabbitai
bot
requested review from
KarryCharon,
PabloWiedemann,
comfydesigner and
shinshin86
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/extensions/core/load3d/Load3dUtils.ts (1)
86-92:uploadMultipleFilesdoesn't report partial failures.The function calls
uploadFilefor each file and usesPromise.all, butuploadFilereturnsundefinedon failure rather than throwing. This means some uploads could fail silently while others succeed, with no aggregated feedback to the caller.Consider returning the results array or at least logging/reporting which files failed:
♻️ Suggested improvement
- static async uploadMultipleFiles(files: FileList, subfolder: string = '3d') { + static async uploadMultipleFiles(files: FileList, subfolder: string = '3d'): Promise<string[]> { const uploadPromises = Array.from(files).map((file) => this.uploadFile(file, subfolder) ) - await Promise.all(uploadPromises) + const results = await Promise.all(uploadPromises) + return results.filter((path): path is string => path !== undefined) }
🤖 Fix all issues with AI agents
In `@src/platform/assets/services/uploadService.test.ts`:
- Around line 57-76: The test "uploads dataURL successfully" sets global.fetch
directly which can leak into other tests; update the test to restore the
global.fetch mock after the test (or replace the direct assignment with
vi.spyOn(global, 'fetch') so it can be auto-restored), and ensure cleanup by
calling vi.restoreAllMocks() or explicitly resetting global.fetch in afterEach/
finally; reference the test case name (it('uploads dataURL successfully'...)),
the global.fetch mock, and the uploadMedia call so the change is applied in that
test file.
- Around line 162-186: The test for uploadMediaBatch is fragile because it
returns the same mockResponse object for multiple fetchApi calls and relies on
json.mockResolvedValueOnce sequencing; fix by making api.fetchApi return
distinct response objects (or mockResolvedValueOnce with separate objects) so
each call has its own json mock resolution—update the test to mock
vi.mocked(api.fetchApi).mockResolvedValueOnce(response1).mockResolvedValueOnce(response2)
or create two mockResponse instances with their own json mocks and have fetchApi
return them, ensuring uploadMediaBatch sees separate responses for each File.
In `@src/platform/assets/services/uploadService.ts`:
- Around line 23-30: The UploadResult interface currently uses response: any;
replace that with a safer type (preferably a defined API response interface like
UploadApiResponse or at minimum response: unknown) in the UploadResult
declaration so callers must explicitly validate/cast the shape; update any
functions that construct or read UploadResult (e.g., places returning
UploadResult or reading .response) to either map the API payload into the new
UploadApiResponse type or perform type guards before accessing fields, and
add/export the new UploadApiResponse type next to UploadResult to keep typings
consistent across the module.
- Around line 46-51: The code currently assumes source is a valid data URL and
calls fetch(source) which can throw for malformed strings; update the upload
logic around the blob creation (the fetch(source) call and subsequent new
File([...]) using source, filename, mimeType) to first validate source (e.g.,
ensure typeof source === 'string' and it matches a data URL pattern such as
starting with "data:") and then wrap the fetch(...) in a try/catch; on error,
throw or return a descriptive error message that includes the invalid source
context so callers can handle malformed input instead of crashing at runtime.
- Around line 17-21: Remove the local ImageRef interface declaration in
uploadService.ts and instead import the exported ImageRef type from
src/stores/maskEditorDataStore.ts; update the top-level imports to include
ImageRef and delete the redundant local interface definition. Ensure usages in
this file (e.g., any references to originalRef or functions that accept
ImageRef) rely on the imported type and that the optional subfolder and type
fields are handled consistently with the store's definition. Confirm there are
no other duplicate type declarations introduced by this change.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/extensions/core/load3d/Load3dUtils.ts`:
- Around line 12-24: Replace the hard-coded temp-upload error string in the temp
upload path of Load3dUtils (the block that calls uploadMedia and checks
result.success) with a localized message using
t('toastMessages.tempUploadFailed', { error: result.error || '' }) and pass that
localized string to useToastStore().addAlert and the thrown Error; follow the
same pattern used in uploadFile() for consistency. Also add the key
"toastMessages.tempUploadFailed" to src/locales/en/main.json with an appropriate
English message that accepts an {error} interpolation. Ensure you import or have
access to the t function in Load3dUtils if not already available.
In `@src/platform/assets/services/uploadService.test.ts`:
- Around line 21-30: Replace the seven occurrences of the unsafe "as any" cast
in this test file by casting the mock fetch Response objects to
"Partial<Response> as Response" so the mocks remain type-safe; locate the
mockResponse objects and the vi.mocked(api.fetchApi).mockResolvedValue(...)
calls (e.g., the mockResponse used in uploadService tests) and change each mock
object cast from "as any" to "as Partial<Response> as Response", and do the same
pattern for other similar mocked responses in this file.
In `@src/platform/assets/services/uploadService.ts`:
- Around line 52-55: The error thrown when a Data URL fails validation in
uploadService.ts currently echoes part of the input (source.substring...), which
can leak user data; update the failure path in the validation block that calls
isDataURL(source) to throw a generic error message (e.g., "Invalid data URL")
without including any portion of the source string, and ensure any callers of
this validation (the upload handler in the same service or functions that
reference source) continue to receive and handle the generic error.
- Around line 18-23: Add runtime validation for the API responses by creating a
Zod schema that matches the UploadApiResponse interface (e.g., const
uploadApiResponseSchema = z.object({ name: z.string(), subfolder:
z.string().optional(), type: z.string().optional() })), then replace raw
resp.json() parsing in uploadService with schema.safeParse(respJson) where
respJson is the parsed JSON; if safeParse fails, return the failure state
(success: false or equivalent) and log/propagate the error consistent with
assetService.ts pattern (use the same error handling as in assetService.ts lines
484-491). Apply this at both places where resp.json() is used in this file so
validated data is returned when safeParse succeeds and failure is returned when
it doesn't.
Comment on lines
12
to
52
| const filename = `${prefix}_${Date.now()}.${fileType}` | ||
| const result = await uploadMedia( | ||
| { source: imageData, filename }, | ||
| { subfolder: 'threed', type: 'temp' } | ||
| ) | ||
|
|
||
| if (resp.status !== 200) { | ||
| const err = `Error uploading temp file: ${resp.status} - ${resp.statusText}` | ||
| if (!result.success) { | ||
| const err = `Error uploading temp file: ${result.error}` | ||
| useToastStore().addAlert(err) | ||
| throw new Error(err) | ||
| } | ||
|
|
||
| return await resp.json() | ||
| return result.response |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and check its contents
fd "Load3dUtils.ts" --type fRepository: Comfy-Org/ComfyUI_frontend
Length of output: 111
🏁 Script executed:
# Check the structure of src directory
git ls-files src/ | head -30Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1127
🏁 Script executed:
# Look for useToastStore to understand if it displays user-facing messages
rg "useToastStore" -t ts -t vue -A 2 --max-count 5Repository: Comfy-Org/ComfyUI_frontend
Length of output: 97
🏁 Script executed:
# Check how error messages are handled in similar upload functions
rg "uploadMedia\|uploadFile" -t ts -A 5 | head -50Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
# Check existing i18n patterns for errors
rg "toastMessages\|toast" src/locales/ -t json | head -20Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
# Read the file to verify the code snippet
cat -n src/extensions/core/load3d/Load3dUtils.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 3399
🏁 Script executed:
# Find useToastStore definition
rg "useToastStore" -t ts --no-heading -B 2 -A 5Repository: Comfy-Org/ComfyUI_frontend
Length of output: 50382
🏁 Script executed:
# Check locales structure
cat src/locales/en/main.json | head -100Repository: Comfy-Org/ComfyUI_frontend
Length of output: 3898
🏁 Script executed:
# Search for similar toast/alert error patterns
rg "addAlert|useToastStore" --type-listRepository: Comfy-Org/ComfyUI_frontend
Length of output: 459
🏁 Script executed:
# Check for toastMessages section in locales
rg "toastMessages" src/locales/en/main.json -A 5 | head -50Repository: Comfy-Org/ComfyUI_frontend
Length of output: 363
🏁 Script executed:
# Look for existing upload-related error keys
rg "upload|Upload" src/locales/en/main.jsonRepository: Comfy-Org/ComfyUI_frontend
Length of output: 1845
🏁 Script executed:
# Check the audioService.ts to see similar pattern
grep -n "Error uploading temp file" src/services/audioService.ts -B 2 -A 2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 262
Localize the temp-upload error message.
The error string on line 19 is user-facing and should be localized. Use t('toastMessages.tempUploadFailed', { error: result.error || '' }) and add the corresponding key to src/locales/en/main.json. This matches the pattern used in uploadFile() (lines 38, 51) and aligns with the coding guideline that error messages must be user-friendly and actionable.
🤖 Prompt for AI Agents
In `@src/extensions/core/load3d/Load3dUtils.ts` around lines 12 - 24, Replace the
hard-coded temp-upload error string in the temp upload path of Load3dUtils (the
block that calls uploadMedia and checks result.success) with a localized message
using t('toastMessages.tempUploadFailed', { error: result.error || '' }) and
pass that localized string to useToastStore().addAlert and the thrown Error;
follow the same pattern used in uploadFile() for consistency. Also add the key
"toastMessages.tempUploadFailed" to src/locales/en/main.json with an appropriate
English message that accepts an {error} interpolation. Ensure you import or have
access to the t function in Load3dUtils if not already available.
Comment on lines
21
to
32
| const mockResponse = { | ||
| status: 200, | ||
| json: vi.fn().mockResolvedValue({ | ||
| name: 'test.png', | ||
| subfolder: 'uploads' | ||
| }) | ||
| } | ||
|
|
||
| vi.mocked(api.fetchApi).mockResolvedValue(mockResponse as any) | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the test file
find . -name "uploadService.test.ts" -type fRepository: Comfy-Org/ComfyUI_frontend
Length of output: 196
🏁 Script executed:
# Read the file with line numbers to see the context
if [ -f "src/platform/assets/services/uploadService.test.ts" ]; then
wc -l "src/platform/assets/services/uploadService.test.ts"
fiRepository: Comfy-Org/ComfyUI_frontend
Length of output: 124
🏁 Script executed:
# Get the specific lines mentioned (21-30) with context
if [ -f "src/platform/assets/services/uploadService.test.ts" ]; then
sed -n '1,40p' "src/platform/assets/services/uploadService.test.ts" | cat -n
fiRepository: Comfy-Org/ComfyUI_frontend
Length of output: 1446
🏁 Script executed:
# Search for all 'as any' occurrences in the file
if [ -f "src/platform/assets/services/uploadService.test.ts" ]; then
rg "as any" "src/platform/assets/services/uploadService.test.ts" -A 2 -B 2
fiRepository: Comfy-Org/ComfyUI_frontend
Length of output: 993
🏁 Script executed:
# Check what Response type is imported or used in the file
if [ -f "src/platform/assets/services/uploadService.test.ts" ]; then
head -30 "src/platform/assets/services/uploadService.test.ts"
fiRepository: Comfy-Org/ComfyUI_frontend
Length of output: 797
🏁 Script executed:
# Read more of the file to see all imports
head -50 "src/platform/assets/services/uploadService.test.ts"Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1406
🏁 Script executed:
# Check the uploadService.ts file to understand return types
cat -n "src/platform/assets/services/uploadService.ts" | head -60Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1877
🏁 Script executed:
# Check what api.fetchApi's type signature is
find . -path "*/scripts/api*" -name "*.ts" -o -name "*.js" | head -10Repository: Comfy-Org/ComfyUI_frontend
Length of output: 421
🏁 Script executed:
# Search for other test patterns with mocked responses in similar test files
rg "mockResolvedValue" "src/**/*.test.ts" -B 5 -A 1 | head -80Repository: Comfy-Org/ComfyUI_frontend
Length of output: 126
🏁 Script executed:
# Check the api.ts file to see fetchApi signature
head -100 "src/scripts/api.ts" | cat -nRepository: Comfy-Org/ComfyUI_frontend
Length of output: 3601
🏁 Script executed:
# Search for fetchApi definition and its return type
rg "fetchApi" "src/scripts/api.ts" -A 10 | head -50Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1658
🏁 Script executed:
# Look for other mock patterns in test files
rg "mockResolvedValue" "src" -A 2 -B 2 | head -100Repository: Comfy-Org/ComfyUI_frontend
Length of output: 8346
🏁 Script executed:
# Check if Response type is used anywhere or if a custom type is defined
rg "interface.*Response|type.*Response" "src/platform/assets/services" -A 5Repository: Comfy-Org/ComfyUI_frontend
Length of output: 430
Replace as any casts in mock responses throughout the test file.
as any violates the project's type-safety guidance. Use as Partial<Response> as Response instead to explicitly acknowledge the incomplete implementation while maintaining type safety. There are 7 occurrences of this pattern in the file that should be updated.
🧩 Example fix
const mockResponse = {
status: 200,
json: vi.fn().mockResolvedValue({
name: 'test.png',
subfolder: 'uploads'
})
}
- vi.mocked(api.fetchApi).mockResolvedValue(mockResponse as any)
+ vi.mocked(api.fetchApi).mockResolvedValue(
+ mockResponse as Partial<Response> as Response
+ )🤖 Prompt for AI Agents
In `@src/platform/assets/services/uploadService.test.ts` around lines 21 - 30,
Replace the seven occurrences of the unsafe "as any" cast in this test file by
casting the mock fetch Response objects to "Partial<Response> as Response" so
the mocks remain type-safe; locate the mockResponse objects and the
vi.mocked(api.fetchApi).mockResolvedValue(...) calls (e.g., the mockResponse
used in uploadService tests) and change each mock object cast from "as any" to
"as Partial<Response> as Response", and do the same pattern for other similar
mocked responses in this file.
Comment on lines
+18
to
+23
| interface UploadApiResponse { | ||
| name: string | ||
| subfolder?: string | ||
| type?: string | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Add runtime validation to uploadService using safeParse for API responses.
The UploadApiResponse interface at lines 18-23 defines the expected shape, but resp.json() is not validated at runtime. Unexpected payloads will bypass the interface and still return success: true, propagating invalid paths. Add a Zod schema with safeParse() validation (following the pattern in assetService.ts lines 484-491) and return a failure state when validation fails.
Implementation pattern
+import { z } from 'zod'
+
+const uploadApiResponseSchema = z.object({
+ name: z.string(),
+ subfolder: z.string().optional(),
+ type: z.string().optional()
+})
...
- const data: UploadApiResponse = await resp.json()
+ const parsed = uploadApiResponseSchema.safeParse(await resp.json())
+ if (!parsed.success) {
+ return {
+ success: false,
+ path: '',
+ name: '',
+ subfolder: '',
+ error: 'Invalid upload response',
+ response: null
+ }
+ }
+ const data: UploadApiResponse = parsed.dataApply this validation at both locations where responses are parsed (lines 18-23 region and lines 128-137).
🤖 Prompt for AI Agents
In `@src/platform/assets/services/uploadService.ts` around lines 18 - 23, Add
runtime validation for the API responses by creating a Zod schema that matches
the UploadApiResponse interface (e.g., const uploadApiResponseSchema =
z.object({ name: z.string(), subfolder: z.string().optional(), type:
z.string().optional() })), then replace raw resp.json() parsing in uploadService
with schema.safeParse(respJson) where respJson is the parsed JSON; if safeParse
fails, return the failure state (success: false or equivalent) and log/propagate
the error consistent with assetService.ts pattern (use the same error handling
as in assetService.ts lines 484-491). Apply this at both places where
resp.json() is used in this file so validated data is returned when safeParse
succeeds and failure is returned when it doesn't.
Comment on lines
52
to
55
| // dataURL string | ||
| if (!isDataURL(source)) { | ||
| throw new Error(`Invalid data URL: ${source.substring(0, 50)}...`) | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid echoing data URL contents in error messages.
Including a substring of the data URL can leak user content into logs/toasts; keep this generic.
🔒 Suggested fix
- if (!isDataURL(source)) {
- throw new Error(`Invalid data URL: ${source.substring(0, 50)}...`)
- }
+ if (!isDataURL(source)) {
+ throw new Error('Invalid data URL')
+ }📝 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
| // dataURL string | |
| if (!isDataURL(source)) { | |
| throw new Error(`Invalid data URL: ${source.substring(0, 50)}...`) | |
| } | |
| // dataURL string | |
| if (!isDataURL(source)) { | |
| throw new Error('Invalid data URL') | |
| } |
🤖 Prompt for AI Agents
In `@src/platform/assets/services/uploadService.ts` around lines 52 - 55, The
error thrown when a Data URL fails validation in uploadService.ts currently
echoes part of the input (source.substring...), which can leak user data; update
the failure path in the validation block that calls isDataURL(source) to throw a
generic error message (e.g., "Invalid data URL") without including any portion
of the source string, and ensure any callers of this validation (the upload
handler in the same service or functions that reference source) continue to
receive and handle the generic error.
christian-byrne
force-pushed
the
refactor/consolidate-image-upload
branch
from
48ca2b8 to
5fc3cb6
Compare
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 26 kB (baseline 26 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 974 kB (baseline 974 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 80.7 kB (baseline 80.7 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 471 kB (baseline 471 kB) • 🟢 -8 BConfiguration panels, inspectors, and settings screens
Status: 12 added / 12 removed User & Accounts — 3.94 kB (baseline 3.94 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 3 added / 3 removed Editors & Dialogs — 2.89 kB (baseline 2.89 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 2 added / 2 removed UI Components — 33.7 kB (baseline 33.7 kB) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed Data & Services — 2.71 MB (baseline 2.71 MB) • 🔴 +1.99 kBStores, services, APIs, and repositories
Status: 8 added / 8 removed Utilities & Hooks — 25.3 kB (baseline 25.3 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 7 added / 7 removed Vendor & Third-Party — 10.7 MB (baseline 10.7 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 7.1 MB (baseline 7.1 MB) • 🟢 -407 BBundles that do not match a named category
Status: 36 added / 36 removed |
🔧 Auto-fixes AppliedThis PR has been automatically updated to fix linting and formatting issues.
Changes made:
|
Contributor
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)
src/extensions/core/load3d/Load3dUtils.ts (1)
142-162: Consider migratinguploadThumbnailto use the shareduploadMediaservice.This method still uses direct
api.fetchApi('/upload/image')with manual FormData construction, which is the pattern the PR aims to consolidate. For consistency, consider refactoring to useuploadMedia:static async uploadThumbnail( 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 + const result = await uploadMedia( + { source: imageData, filename }, + { subfolder, type: type as ResultItemType } + ) + return result.success }If there's a specific reason to keep the direct implementation (e.g., the
typeparameter difference), please document it.
🤖 Fix all issues with AI agents
In
`@src/renderer/extensions/vueNodes/widgets/components/WidgetSelectDropdown.vue`:
- Around line 251-274: In uploadFiles, avoid calling assetsStore.updateInputs()
inside the per-file upload loop (uploadPromises) because it triggers redundant
updates; instead call assetsStore.updateInputs() once after awaiting
Promise.all(results) when folder === 'input'. Locate the uploadFiles function
and move the updateInputs() call out of the files.map async callback to run
conditionally after the Promise.all completes, or refactor to use
uploadMediaBatch (mapping files to { source: file } and passing { type: folder
}) and then call assetsStore.updateInputs() once if folder === 'input'.
♻️ Duplicate comments (3)
src/extensions/core/load3d/Load3dUtils.ts (1)
44-47: Localize the temp-upload error message.The error string on line 45 is user-facing and should be localized. Use the i18n
t()function with a key liket('toastMessages.tempUploadFailed', { error: result.error || '' })and add the corresponding key tosrc/locales/en/main.json. This aligns with the pattern used inuploadFile()(lines 64-67, 77) where localized messages are used.🐛 Proposed fix
if (!result.success || !result.response) { - const err = `Error uploading temp file: ${result.error}` + const err = t('toastMessages.tempUploadFailed', { error: result.error || '' }) useToastStore().addAlert(err) throw new Error(err) }And add to
src/locales/en/main.json:"tempUploadFailed": "Error uploading temp file: {error}"src/platform/assets/services/uploadService.ts (1)
52-55: Avoid echoing data URL contents in error messages.Including a substring of the data URL can leak user content into logs/toasts. Keep this generic.
🔒 Suggested fix
// dataURL string if (!isDataURL(source)) { - throw new Error(`Invalid data URL: ${source.substring(0, 50)}...`) + throw new Error('Invalid data URL') }src/platform/assets/services/uploadService.test.ts (1)
19-37: Replaceas anycast with safer type assertion.The
as anyon line 29 violates type safety guidelines. Useas Partial<Response> as Responseto explicitly acknowledge the incomplete mock while maintaining type safety.♻️ Suggested fix
- vi.mocked(api.fetchApi).mockResolvedValue(mockResponse as any) + vi.mocked(api.fetchApi).mockResolvedValue( + mockResponse as Partial<Response> as Response + )Apply this pattern to all 7 occurrences in the file (lines 29, 49, 71, 97, 134, 160, 197, 198).
src/renderer/extensions/vueNodes/widgets/components/WidgetSelectDropdown.vue
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/renderer/extensions/vueNodes/widgets/components/WidgetSelectDropdown.vue`:
- Around line 261-266: The fallback string 'Upload failed' in
WidgetSelectDropdown.vue should be localized: add a key (e.g. "upload.failed")
to src/locales/en/main.json and replace the hardcoded fallback used in the
failedUploads handling (the results/filter block that calls toastStore.addAlert)
with a vue-i18n lookup (via this.$t or useI18n().t) so toastStore.addAlert
receives the translated message (e.g., toastStore.addAlert(t('upload.failed'))
when result.error is absent); ensure the new key is added to other locale files
as needed.
Comment on lines
261
to
266
| // Collect failed uploads for error reporting | ||
| const failedUploads = results.filter((result) => !result.success) | ||
| if (failedUploads.length > 0) { | ||
| failedUploads.forEach((result) => { | ||
| toastStore.addAlert(result.error || 'Upload failed') | ||
| }) |
Contributor
There was a problem hiding this comment.
Localize the upload failure toast fallback.
The fallback string is user-facing and should use vue-i18n. Consider adding a translation key in src/locales/en/main.json.
✅ Suggested change
- toastStore.addAlert(result.error || 'Upload failed')
+ toastStore.addAlert(
+ result.error || t('widgets.uploadSelect.uploadFailed')
+ )🤖 Prompt for AI Agents
In `@src/renderer/extensions/vueNodes/widgets/components/WidgetSelectDropdown.vue`
around lines 261 - 266, The fallback string 'Upload failed' in
WidgetSelectDropdown.vue should be localized: add a key (e.g. "upload.failed")
to src/locales/en/main.json and replace the hardcoded fallback used in the
failedUploads handling (the results/filter block that calls toastStore.addAlert)
with a vue-i18n lookup (via this.$t or useI18n().t) so toastStore.addAlert
receives the translated message (e.g., toastStore.addAlert(t('upload.failed'))
when result.error is absent); ensure the new key is added to other locale files
as needed.
Creates core uploadService to eliminate ~60-70 LOC duplication across multiple implementations. Changes: - Add src/platform/assets/services/uploadService.ts with uploadMedia() and uploadMediaBatch() - Refactor Load3dUtils to use uploadService (eliminates 50+ LOC) - Refactor WidgetSelectDropdown to use uploadService (eliminates 20+ LOC) - Add comprehensive unit tests for uploadService - Maintain backward compatibility for all existing APIs Benefits: - Single source of truth for upload logic - Consistent error handling - Type-safe interfaces - Easier to test and maintain
- Import ImageRef from maskEditorDataStore instead of duplicating - Replace 'any' with proper UploadApiResponse type - Add validation for dataURL strings - Fix test mocking: use vi.spyOn for global.fetch - Fix uploadMediaBatch test to use distinct response mocks - Add test for invalid dataURL rejection - Fix uploadMultipleFiles to return array of successful paths - Optimize file size test to avoid timeout
- Add back api import to Load3dUtils (needed by fileExists and uploadThumbnail) - Add null check for result.response in uploadTempImage - Ensures type safety after rebase on main
- Remove data URL content from error messages (security) - Replace 'as any' casts with proper type assertions in tests - Use uploadMediaBatch in WidgetSelectDropdown for single store update - Localize error strings with i18n (uploadFailed, tempUploadFailed) Amp-Thread-ID: https://ampcode.com/threads/T-019c0daa-eb18-72eb-bc87-90f09afc3d3a Co-authored-by: Amp <[email protected]>
christian-byrne
force-pushed
the
refactor/consolidate-image-upload
branch
from
7b7f70d to
bc0697b
Compare
AustinMroz
approved these changes
Jan 30, 2026
Collaborator
AustinMroz
left a comment
AustinMroz
left a comment
There was a problem hiding this comment.
Thought we'd be seeing more red than green here, but consolidating the flow of logic still makes changes easier in the future.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Consolidates duplicate image upload implementations into a shared service, eliminating ~60-70 LOC duplication. RFC here.
Changes
src/platform/assets/services/uploadService.tswithuploadMedia()anduploadMediaBatch()Load3dUtilsto use uploadService (eliminates 50+ LOC)WidgetSelectDropdownto use uploadService (eliminates 20+ LOC)Benefits
┆Issue is synchronized with this Notion page by Unito