Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 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
55 changes: 55 additions & 0 deletions .github/workflows/pr-cursor-review.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
# Description: Team-gated multi-model Cursor review — a thin caller for the
# reusable workflow in Comfy-Org/github-workflows, which is the single source of
# truth for the panel, judge, prompts, and scripts. Triggered by the
# 'cursor-review' label.
#
# Access control (team-only, two layers):
# 1. Only users with triage permission or higher can apply a label in a public
# repo, so the public cannot trigger this.
# 2. The reusable workflow's secret-bearing jobs do not run on fork PRs (forks
# get no secrets), so CURSOR_API_KEY is reachable only on internal branches.
name: 'PR: Cursor Review'

on:
pull_request:
types: [labeled, unlabeled]

permissions:
contents: read
pull-requests: write

concurrency:
# Re-labeling cancels an in-flight run for the same PR + label.
group: cursor-review-pr-${{ github.event.pull_request.number }}-${{ github.event.label.name }}
cancel-in-progress: true

jobs:
cursor-review:
if: github.event.action == 'labeled' && github.event.label.name == 'cursor-review'
# SHA-pinned per zizmor `unpinned-uses: hash-pin`. Bump this SHA to pick up
# upstream changes; keep `workflows_ref` matching so prompts/scripts load
# from the same commit as the workflow definition.
uses: Comfy-Org/github-workflows/.github/workflows/cursor-review.yml@047ca48febe3a6647608ed2e0c4331b491cb9d6a # github-workflows#9
with:
# Overriding diff_excludes replaces the reusable default wholesale, so
# this restates the generated/vendored defaults and adds this repo's heavy
# paths (Playwright snapshots, generated manager types).
diff_excludes: >-
:!**/package-lock.json
:!**/yarn.lock
:!**/pnpm-lock.yaml
:!**/node_modules/**
:!**/.claude/**
:!**/dist/**
:!**/vendor/**
:!**/*.generated.*
:!**/*.min.js
:!**/*.min.css
:!**/*-snapshots/**
:!src/workbench/extensions/manager/types/generatedManagerTypes.ts
# Load the prompts/scripts from the same ref as `uses:`.
workflows_ref: 047ca48febe3a6647608ed2e0c4331b491cb9d6a
secrets:
CURSOR_API_KEY: ${{ secrets.CURSOR_API_KEY }}
# Optional — enables start/complete Slack DMs to the triggerer.
SLACK_BOT_TOKEN: ${{ secrets.SLACK_BOT_TOKEN }}
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<script setup lang="ts">
import { useDebounceFn } from '@vueuse/core'
import { computed, provide, ref, toRef } from 'vue'
import { useI18n } from 'vue-i18n'

Expand Down Expand Up @@ -150,6 +151,15 @@ function handleIsOpenUpdate(isOpen: boolean) {
void outputMediaAssets.refresh()
}
}

const handleApproachEnd = useDebounceFn(async () => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

question (non-blocking): per the PR description, VueUse ≥14's throttleFilter(leading=false) makes useScroll's throttle: 64 in VirtualGrid drop discrete mouse-wheel events, so approach-end only fires for high-frequency (trackpad-style) scrolling. That leaves this wiring effectively inert for mouse-wheel users until the separate throttle bug is fixed. Is the intent to land the (harmless-but-inactive) wiring ahead of that fix, or to stack the throttle fix first so FE-988 ships actually-working for everyone? Either is defensible — just flagging that on merge the feature is non-functional for a large share of users.

@mattmillerai mattmillerai Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Confirmed on the wiring side: @approach-end is debounced 300ms and guarded by hasMore/isLoadingMore before loadMore(), with :loading-more driving the spinner — correct in isolation and a no-op when approach-end never fires. Your premise holds too: the event originates upstream in VirtualGrid's useScroll({ throttle: 64 }), and the leading=false wheel-event drop lives there, outside this diff.

So this lands as-is: the mouse-wheel dormancy is a pre-existing VirtualGrid throttle limitation that's tracked separately — not introduced or worsened by this PR, and trackpad scrolling works today. Not gating this change.

Comment thread
mattmillerai marked this conversation as resolved.
if (
outputMediaAssets.hasMore.value &&
!outputMediaAssets.isLoadingMore.value
Comment thread
mattmillerai marked this conversation as resolved.
) {
await outputMediaAssets.loadMore()
}
}, 300)
</script>

<template>
Expand All @@ -172,10 +182,12 @@ function handleIsOpenUpdate(isOpen: boolean) {
:show-base-model-filter
:base-model-options
v-bind="combinedProps"
:loading-more="outputMediaAssets.isLoadingMore.value"
class="w-full"
@update:selected="updateSelectedItems"
@update:files="handleFilesUpdate"
@update:is-open="handleIsOpenUpdate"
@approach-end="handleApproachEnd"
/>
</WidgetLayoutField>
</template>
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ interface Props {
ownershipOptions?: OwnershipFilterOption[]
showBaseModelFilter?: boolean
baseModelOptions?: FilterOption[]
loadingMore?: boolean
isSelected?: (
selected: Set<string>,
item: FormDropdownItem,
Expand All @@ -63,11 +64,16 @@ const {
ownershipOptions,
showBaseModelFilter,
baseModelOptions,
loadingMore = false,
isSelected = (selected, item, _index) => selected.has(item.id),
searcher = defaultSearcher,
items
} = defineProps<Props>()

const emit = defineEmits<{
(e: 'approach-end'): void
}>()

const placeholderText = computed(
() => placeholder ?? t('widgets.uploadSelect.placeholder')
)
Expand Down Expand Up @@ -314,9 +320,11 @@ function handleSearchEnter() {
:candidate-label
:is-selected="internalIsSelected"
:max-selectable
:loading-more="loadingMore"
@close="closeDropdown"
@search-enter="handleSearchEnter"
@item-click="handleSelection"
@approach-end="emit('approach-end')"
/>
</Popover>
</div>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import userEvent from '@testing-library/user-event'
import { render, screen } from '@testing-library/vue'
import { describe, expect, it } from 'vitest'

Expand All @@ -7,8 +8,9 @@ import type { FormDropdownItem, LayoutMode } from './types'
const VirtualGridStub = {
name: 'VirtualGrid',
props: ['items', 'maxColumns', 'itemHeight', 'scrollerHeight'],
emits: ['approach-end'],
template:
'<div data-testid="virtual-grid" :data-items="JSON.stringify(items)" :data-max-columns="maxColumns" />'
'<div data-testid="virtual-grid" :data-items="JSON.stringify(items)" :data-max-columns="maxColumns" @click="$emit(\'approach-end\')" />'
}

function createItem(id: string, name: string): FormDropdownItem {
Expand Down Expand Up @@ -93,6 +95,31 @@ describe('FormDropdownMenu', () => {
expect(virtualGrid.getAttribute('data-max-columns')).toBe('1')
})

it('forwards approach-end from the virtual grid', async () => {
const user = userEvent.setup()
const { emitted } = render(FormDropdownMenu, {
props: defaultProps,
global: globalConfig
})

await user.click(screen.getByTestId('virtual-grid'))

expect(emitted()['approach-end']).toHaveLength(1)
})

it('shows the loading-more row only while loadingMore is set', async () => {
const { rerender } = render(FormDropdownMenu, {
props: { ...defaultProps, loadingMore: true },
global: globalConfig
})

expect(screen.getByTestId('form-dropdown-loading-more')).toBeTruthy()

await rerender({ ...defaultProps, loadingMore: false })

expect(screen.queryByTestId('form-dropdown-loading-more')).toBeNull()
})

it('has data-capture-wheel="true" on the root element', () => {
render(FormDropdownMenu, {
props: defaultProps,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ interface Props {
baseModelOptions?: FilterOption[]
candidateIndex?: number
candidateLabel?: string
loadingMore?: boolean
}

const {
Expand All @@ -39,11 +40,13 @@ const {
showBaseModelFilter,
baseModelOptions,
candidateIndex = -1,
candidateLabel
candidateLabel,
loadingMore = false
} = defineProps<Props>()
const emit = defineEmits<{
(e: 'item-click', item: FormDropdownItem, index: number): void
(e: 'search-enter'): void
(e: 'approach-end'): void
}>()

const filterSelected = defineModel<string>('filterSelected')
Expand Down Expand Up @@ -158,6 +161,7 @@ const onWheel = (event: WheelEvent) => {
:default-item-width="layoutConfig.itemWidth"
:buffer-rows="2"
class="mt-2 min-h-0 flex-1"
@approach-end="emit('approach-end')"
>
<template #item="{ item, index }">
<FormDropdownMenuItem
Expand All @@ -172,5 +176,15 @@ const onWheel = (event: WheelEvent) => {
/>
</template>
</VirtualGrid>
<div
v-if="loadingMore"
class="flex items-center justify-center py-2"
data-testid="form-dropdown-loading-more"
>
<i
:aria-label="$t('g.loading')"
class="icon-[lucide--loader] size-6 animate-spin text-muted-foreground"
/>
</div>
</div>
</template>
Loading
Loading