-
Notifications
You must be signed in to change notification settings - Fork 414
fix: dropdown widget fetching output files #6734
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -67,15 +67,15 @@ const searchQuery = defineModel<string>('searchQuery') | |||
| " | ||||
| > | ||||
| <div class="pointer-events-none absolute inset-x-3 top-0 z-10 h-5" /> | ||||
| <div | ||||
| v-if="items.length === 0" | ||||
| class="absolute inset-0 flex items-center justify-center" | ||||
| > | ||||
| <i | ||||
| title="No items" | ||||
| class="icon-[lucide--circle-off] size-30 text-zinc-500/20" | ||||
| /> | ||||
| </div> | ||||
| <template v-if="items.length === 0"> | ||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [quality] low Priority Issue: Template structure change from div to template is good but missing semantic improvement |
||||
| <div class="absolute inset-0 flex items-center justify-center"> | ||||
| <i | ||||
| title="No items" | ||||
| class="icon-[lucide--circle-off] size-30 text-zinc-500/20" | ||||
| /> | ||||
| </div> | ||||
|
Comment on lines
+71
to
+76
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: We also have this
(not blocking this PR). |
||||
| <div class="min-h-50" /> | ||||
| </template> | ||||
| <!-- Item --> | ||||
| <FormDropdownMenuItem | ||||
| v-for="(item, index) in items" | ||||
|
|
||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
outputItemsguard prevents model outputs from ever showingIn
outputItems, this guard:short-circuits for
assetKind === 'model', even thoughtoAssertTypeexplicitly maps a'3D'media type to'model'. That means 3D/model outputs will never appear in the dropdown, despite having logic to support them.A minimal fix is to include
'model'in the allowed list so those outputs can be surfaced:That keeps the filtering behavior but actually allows model/3D outputs to show up. If
assetKind: 'unknown'should also see outputs, you may want to relax the guard further.Also applies to: 145-158, 340-346
I'll help you verify the review comment about the
WidgetSelectDropdown.vuefile. Let me examine the code and understand the context.Length of output: 5887
🏁 Script executed:
Length of output: 27
🏁 Script executed:
Length of output: 27
🏁 Script executed:
Length of output: 1553
Length of output: 311
🏁 Script executed:
Length of output: 240
🏁 Script executed:
Length of output: 1047
Fix guards to allow model outputs to display
The
outputItemscomputed property currently returns empty for anyassetKindnot in['image', 'video', 'audio']. However,toAssertTypeexplicitly maps the'3D'media type to'model'(line 145), andAssetKindtype definition includes'model'as valid, yet the guard at line 124 prevents model outputs from ever being processed.Additionally,
getMediaUrl(line 344) has a similar guard that excludes both'audio'and'model'from media URL generation.Required fixes:
'model'to the allowed asset kinds inoutputItemsgetMediaUrlguard to include'audio'and'model'(or make it comprehensive based on intended support)The suggested diff in the original review for line 124 is appropriate. Apply similar logic to the
getMediaUrlguard to ensure model assets can display media previews.🤖 Prompt for AI Agents