-
Notifications
You must be signed in to change notification settings - Fork 490
Fix error on close of combo dropdown #7804
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
Changes from 3 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 |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| <script> | ||
| import Select from 'primevue/select' | ||
|
|
||
| export default { | ||
| name: 'SelectPlus', | ||
| extends: Select, | ||
| emits: ['hide'], | ||
| methods: { | ||
| onOverlayLeave() { | ||
| this.unbindOutsideClickListener() | ||
| this.unbindScrollListener() | ||
| this.unbindResizeListener() | ||
|
|
||
| this.$emit('hide') | ||
| this.overlay = null | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+4
to
+18
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. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# First, check if the file exists and examine its full content
echo "=== Checking SelectPlus.vue file ==="
cat -n src/components/primevueOverride/SelectPlus.vue
# Check PrimeVue Select implementation for onOverlayLeave
echo -e "\n=== Checking PrimeVue Select onOverlayLeave ==="
find node_modules/primevue -name "Select.vue" -o -name "Select.js" 2>/dev/null | head -1 | xargs grep -A 20 "onOverlayLeave" 2>/dev/null || echo "Not found in Select files"
# Search more broadly in primevue for this method
echo -e "\n=== Searching PrimeVue for onOverlayLeave ==="
grep -r "onOverlayLeave" node_modules/primevue --include="*.vue" --include="*.js" 2>/dev/null | head -5
# Check if there's a BaseSelect or mixin
echo -e "\n=== Looking for Select core implementation ==="
ls -la node_modules/primevue/select/ 2>/dev/null || echo "No select directory"Repository: Comfy-Org/ComfyUI_frontend Length of output: 771 🏁 Script executed: #!/bin/bash
# Check PrimeVue version and structure
echo "=== PrimeVue package info ==="
grep '"primevue"' package.json || grep '"primevue"' package-lock.json | head -2
# Look at what methods Select actually has
echo -e "\n=== Checking Select component methods ==="
node -e "const Select = require('primevue/select').default; console.log(Object.getOwnPropertyNames(Select.prototype).sort())"
# Find Select implementation file
echo -e "\n=== Finding Select implementation ==="
find node_modules/primevue -type f -name "*.js" -o -name "*.vue" | grep -i select | head -10
# Check actual Select file
echo -e "\n=== Examining Select implementation ==="
cat node_modules/primevue/select/select.esm.js 2>/dev/null | head -100 || cat node_modules/primevue/dist/select/select.esm.js 2>/dev/null | head -100Repository: Comfy-Org/ComfyUI_frontend Length of output: 1167 🏁 Script executed: #!/bin/bash
# Search for unbindOutsideClickListener usage to understand where it comes from
echo "=== Searching for unbind methods in codebase ==="
rg "unbindOutsideClickListener|unbindScrollListener|unbindResizeListener" src/ --type vue --type ts --type js
# Check if SelectPlus is used elsewhere
echo -e "\n=== Finding SelectPlus usage ==="
rg "SelectPlus|from.*SelectPlus" src/ --type vue --type ts
# Look for other PrimeVue overrides
echo -e "\n=== Finding other PrimeVue overrides ==="
find src/components/primevueOverride -type f -name "*.vue" 2>/dev/null
# Check git history/comments about SelectPlus
echo -e "\n=== Checking for context in related files ==="
grep -r "autofocus" src/components/primevueOverride/ --include="*.vue" --include="*.ts" --include="*.js" -B 2 -A 2 2>/dev/null || echo "No autofocus refs found"
# Search for any parent method calls in the overrides
echo -e "\n=== Looking for super calls in overrides ==="
rg "super\." src/components/primevueOverride/ --type vueRepository: Comfy-Org/ComfyUI_frontend Length of output: 512 🏁 Script executed: #!/bin/bash
# Search without type restrictions
echo "=== Searching for onOverlayLeave ==="
rg "onOverlayLeave" src/
# Search for SelectPlus imports/usage
echo -e "\n=== SelectPlus usage ==="
rg "SelectPlus" src/
# Check PrimeVue docs - look at node_modules structure differently
echo -e "\n=== PrimeVue files ==="
find node_modules -path "*primevue*" -name "*.d.ts" 2>/dev/null | grep -i select | head -5
# Try to find TypeScript definitions for Select
echo -e "\n=== PrimeVue Select types ==="
find . -name "*.d.ts" -exec grep -l "class Select\|interface Select" {} \; 2>/dev/null | head -3 | xargs cat 2>/dev/null | head -80
# Check package.json for primevue version and structure
echo -e "\n=== Full primevue config ==="
rg '"primevue"' . --type json -A 5 -B 2
# Look for pnpm-lock or package-lock for primevue version
echo -e "\n=== Primevue in lock file ==="
grep -A 2 '"primevue":' pnpm-lock.yaml 2>/dev/null | head -10Repository: Comfy-Org/ComfyUI_frontend Length of output: 2516 🏁 Script executed: #!/bin/bash
# Check WidgetSelectDefault to understand SelectPlus usage context
echo "=== WidgetSelectDefault context ==="
cat -n src/renderer/extensions/vueNodes/widgets/components/WidgetSelectDefault.vue | head -80
# Look for recent git history on SelectPlus
echo -e "\n=== Git log for SelectPlus ==="
git log --oneline src/components/primevueOverride/SelectPlus.vue 2>/dev/null | head -5
# Get commit message for SelectPlus
echo -e "\n=== SelectPlus commit details ==="
git log -1 --format="%B" -- src/components/primevueOverride/SelectPlus.vue 2>/dev/null || echo "No git history"
# Check if there are any comments about why this override exists
echo -e "\n=== Comments near SelectPlus in codebase ==="
grep -B 5 -A 2 "SelectPlus" src/renderer/extensions/vueNodes/widgets/components/WidgetSelectDefault.vue 2>/dev/null | head -20
# Look for AutoCompletePlus to see if it has similar pattern
echo -e "\n=== AutoCompletePlus structure ==="
cat src/components/primevueOverride/AutoCompletePlus.vueRepository: Comfy-Org/ComfyUI_frontend Length of output: 4714 Consider calling the parent's The override doesn't invoke the parent implementation, unlike the pattern used in 🤖 Prompt for AI Agents |
||
| </script> | ||
|
Comment on lines
1
to
19
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. 🧹 Nitpick | 🔵 Trivial Consider long-term migration away from PrimeVue components. While this override appropriately uses the Options API for PrimeVue compatibility, note that the codebase is moving toward VueUse, shadcn/vue, or Reka UI instead of PrimeVue components. This is acceptable as a short-term fix for existing usage, but consider planning a migration path. Based on learnings, the repository is transitioning away from PrimeVue components.
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. @coderabbitai Where else in the codebase do we use the Options API for Vue components?
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. 🧩 Analysis chain🏁 Script executed: Length of output: 440 @DrJKL The Options API is used in only 2 places in the codebase, both in the
Both files use the Options API specifically because they extend PrimeVue components, which is consistent with the repository's guideline that states: "Use the Vue 3 Composition API instead of the Options API when writing Vue components (exception: when overriding or extending PrimeVue components for compatibility)." The rest of the codebase uses the Composition API (typically 🧠 Learnings used
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. @coderabbitai Open an issue tracking the removal of SelectPlus and replacing it with our own component. Assign @AustinMroz
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. 🧩 Analysis chain🏁 Script executed: Length of output: 57 @DrJKL I've created issue #8034 to track the removal of SelectPlus and its replacement with a custom component. The issue has been assigned to @AustinMroz as requested. 🧠 Learnings used |
||
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.
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.
I was a little concerned that this change would also result in overriding the emits defined by the extended Select component, but, as far as I can tell, defining it here doesn't change the emitsOptions on the component instance.
Suggestion should be fine to make.