perf: virtualize FormDropdownMenu to reduce DOM nodes and image requests#8476
Open
christian-byrne wants to merge 2 commits intomainfrom
Open
perf: virtualize FormDropdownMenu to reduce DOM nodes and image requests#8476christian-byrne wants to merge 2 commits intomainfrom
christian-byrne wants to merge 2 commits intomainfrom
Conversation
dosubot
bot
added
the
size:L
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReworked dropdown UI surface to use a measured, virtualized Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Menu as FormDropdownMenu
participant Grid as VirtualGrid
participant Viewport as Viewport
participant Item as FormDropdownMenuItem
User->>Menu: open dropdown
Menu->>Menu: compute layoutConfig & gridStyle, map items -> virtualItems
Menu->>Grid: provide virtualItems, keys, gridStyle
Grid->>Viewport: measure container size & scroll (useElementSize/useScroll)
Viewport-->>Grid: sizes & scroll offsets
Grid->>Grid: calculate visible start/end, spacer heights
Grid->>Item: render visible items with { item, index }
User->>Item: interact (click)
Item->>Menu: emit selection (item, index)
Menu-->>User: update selection state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 02/14/2026, 05:02:50 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
|
Playwright: ❌ 521 passed, 2 failed · 1 flaky ❌ Failed Tests📊 Browser Reports
|
coderabbitai
bot
requested review from
AustinMroz,
luke-mino-altherr and
viva-jinyi
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 21.7 kB (baseline 21.7 kB) • 🔴 +6 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 888 kB (baseline 879 kB) • 🔴 +8.31 kBGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 68.9 kB (baseline 68.9 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 427 kB (baseline 427 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed User & Accounts — 16.1 kB (baseline 16.1 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 5 added / 5 removed Editors & Dialogs — 785 B (baseline 785 B) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 1 added / 1 removed UI Components — 40.3 kB (baseline 36.6 kB) • 🔴 +3.72 kBReusable component library chunks
Status: 6 added / 5 removed Data & Services — 2.15 MB (baseline 2.15 MB) • 🔴 +98 BStores, services, APIs, and repositories
Status: 12 added / 12 removed Utilities & Hooks — 237 kB (baseline 237 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 13 added / 13 removed Vendor & Third-Party — 8.69 MB (baseline 8.69 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 7.3 MB (baseline 7.31 MB) • 🟢 -11.8 kBBundles that do not match a named category
Status: 51 added / 52 removed |
![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: 2
🤖 Fix all issues with AI agents
In
`@src/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdownMenu.vue`:
- Around line 79-91: The gridTemplateColumns property in the computed gridStyle
is redundant because VirtualGrid's mergedGridStyle will override it with
repeat(${maxColumns}, minmax(0, 1fr)) when maxColumns is finite (which it always
is here), so remove the gridTemplateColumns key from the gridStyle computed
object (leave display, gap, padding, width intact) to avoid dead code; reference
the gridStyle computed, layoutMode.value conditions, and the
VirtualGrid/mergedGridStyle behavior when making the change.
- Line 30: Replace the non-destructured props assignment const props =
defineProps<Props>() with reactive destructuring so the component accesses props
directly (e.g., const { items, ... } = defineProps<Props>()), then update usages
(notably where items is referenced around the template and the code at the
former line 95) to use items directly; ensure you keep the Props type on
defineProps and preserve reactivity by using the direct destructuring pattern
recommended by the guidelines.
src/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdownMenu.vue
Show resolved
Hide resolved
src/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdownMenu.vue
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/components/common/VirtualGrid.test.ts`:
- Around line 74-79: The test currently skips all assertions when
receivedIndices is empty which masks failures; update the assertion around the
receivedIndices checks in VirtualGrid.test (the block using receivedIndices and
the for loop) to explicitly assert that receivedIndices.length > 0 (e.g.,
expect(receivedIndices.length).toBeGreaterThan(0)) before asserting the first
index and the incremental sequence, and keep the existing loop that verifies
receivedIndices[i] === receivedIndices[i-1] + 1 so the test fails if no items
render or indices are non-sequential.
- Around line 84-101: The test "respects maxColumns prop" in VirtualGrid.test.ts
currently only checks for a grid element but doesn't assert that maxColumns: 2
influences layout; update the test that mounts VirtualGrid<TestItem> to read the
rendered element's computed style (or the element.style.gridTemplateColumns) and
assert it contains exactly two column tracks (e.g., two "px" or "fr" entries or
matches a regex for two columns), referencing the mounted wrapper and the grid
element found via wrapper.find(...) and using
getComputedStyle(gridElement.element).gridTemplateColumns (or
gridElement.element.style.gridTemplateColumns) to verify the maxColumns behavior
before calling wrapper.unmount().
In `@src/components/common/VirtualGrid.vue`:
- Around line 60-62: Remove the explicit defineSlots call (defineSlots<{ item:
(props: { item: T & { key: string }; index: number }) => unknown }>() ) from
VirtualGrid.vue; instead rely on the template-declared slot (the "item" slot
already defined at line 14) for slot typing per the coding guideline, so delete
the defineSlots invocation and ensure any TypeScript generics or props
referenced in the template stay intact to preserve type inference for the item
slot.
Contributor
Author
|
There's some issues right now like the grid not being properly truncated. To confirm: bottleneck is the DOM insertion |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdownMenu.vue (1)
83-84:⚠️ Potential issue | 🟠 MajorContainer needs fixed height for proper virtualization.
The PR objectives mention changing from
max-h-[640px]toh-[640px], but the current code still usesmax-h-[640px]. This likely explains the truncation issue you reported—VirtualGrid needs a deterministic container height to calculate the visible range correctly. Withmax-h, the container can shrink based on content, causing virtualization miscalculations.🐛 Proposed fix
<div - class="flex max-h-[640px] w-103 flex-col rounded-lg bg-component-node-background pt-4 outline outline-offset-[-1px] outline-node-component-border" + class="flex h-[640px] w-103 flex-col rounded-lg bg-component-node-background pt-4 outline outline-offset-[-1px] outline-node-component-border" >
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/components/common/VirtualGrid.test.ts`:
- Around line 23-46: The test can pass with zero rendered items and mask
regressions; after mounting VirtualGrid (in VirtualGrid.test.ts) and computing
renderedItems via wrapper.findAll('.test-item'), add an assertion that at least
one item is rendered (e.g., expect(renderedItems.length).toBeGreaterThan(0))
before the existing virtualization assertion that renderedItems.length is less
than items.length; update references around createItems, wrapper and
renderedItems so the test fails if nothing is rendered.
In `@src/components/common/VirtualGrid.vue`:
- Around line 9-15: The slot binding in VirtualGrid.vue uses an explicit prop
binding for item; update the <slot name="item" ...> usage to use the same-name
shorthand for the item prop (replace :item="item" with :item) while leaving the
other prop (:index="state.start + i") unchanged so it follows the repo's
slot-binding convention and matches existing styling in the component rendering
loop.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/common/VirtualGrid.test.ts`:
- Around line 134-151: The test "renders empty when no items provided" mounts a
wrapper but never unmounts it, which can leak DOM state; add a call to
wrapper.unmount() at the end of this test (after the expect) to tear down the
mounted component instance, or alternatively introduce a scoped variable and an
afterEach(() => wrapper?.unmount()) cleanup; reference the wrapper variable and
the test name "renders empty when no items provided" to locate where to add
wrapper.unmount().
LittleSound
reviewed
Feb 3, 2026
Comment on lines
112
to
135
| @@ -97,7 +131,7 @@ const searchQuery = defineModel<string>('searchQuery') | |||
| :layout="layoutMode" | |||
| @click="emit('item-click', item, index)" | |||
| /> | |||
| </div> | |||
| </div> | |||
| </template> | |||
| </VirtualGrid> | |||
Collaborator
There was a problem hiding this comment.
The tooltip for the file name was not displaying properly when scrolling to the bottom, and I've fixed it.
- Fixes a VirtualGrid edge-case where grid gap wasn’t included in the measured row/column step,
causing scrollTop to jitter near the bottom (e.g. 429.5 ↔ 429) and breaking PrimeVue tooltips
(they auto-hide on scroll). - Now measures the actual grid step via offsetTop/offsetLeft and computes spacer heights by whole
rows for stable layout. - Adds a unit test to lock in “row step includes gap” behavior and prevent regressions.
dosubot
bot
added
size:XL
LittleSound
previously approved these changes
Feb 3, 2026
LittleSound
previously approved these changes
Feb 3, 2026
LittleSound
previously approved these changes
Feb 3, 2026
christian-byrne
commented
Feb 3, 2026
dosubot
bot
added
size:L
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/common/VirtualGrid.vue (1)
104-113:⚠️ Potential issue | 🟠 MajorSpacer height undercounts partial rows, which can truncate scroll range.
rowsToHeight(itemsCount)divides bycols, so any remaining items that don’t fill a full row only contribute a fractional height. In a CSS grid, a partially filled row still consumes a full row height, so the bottom spacer becomes too short and items near the end can become unreachable. This matches the “truncation” symptoms noted in the PR discussion.🛠️ Suggested fix
-function rowsToHeight(rows: number): string { - return `${(rows / cols.value) * itemHeight.value}px` -} +function rowsToHeight(itemsCount: number): string { + const rows = Math.ceil(itemsCount / cols.value) + return `${rows * itemHeight.value}px` +}
🧹 Nitpick comments (1)
src/components/common/VirtualGrid.test.ts (1)
9-23: Avoid module-level mutable refs in the vueuse mock.
mockedWidth/Height/ScrollYare mutated across tests at module scope, which can leak state between cases. Usevi.hoisted()to define them for the mock and reset inbeforeEachto keep tests isolated.♻️ Suggested pattern
-import { describe, expect, it, vi } from 'vitest' +import { beforeEach, describe, expect, it, vi } from 'vitest' import { nextTick, ref } from 'vue' -const mockedWidth = ref(400) -const mockedHeight = ref(200) -const mockedScrollY = ref(0) +const { mockedWidth, mockedHeight, mockedScrollY } = vi.hoisted(() => ({ + mockedWidth: ref(400), + mockedHeight: ref(200), + mockedScrollY: ref(0) +})) + +beforeEach(() => { + mockedWidth.value = 400 + mockedHeight.value = 200 + mockedScrollY.value = 0 +})Based on learnings: "Keep module mocks contained; do not use global mutable state within test files; use
vi.hoisted()if necessary for per-test Arrange phase manipulation".
dosubot
bot
added
size:XXL
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 (2)
src/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdown.vue (1)
144-144:⚠️ Potential issue | 🟡 MinorUse i18n for user-facing toast message.
The hardcoded string
"Maximum selection limit reached"should use vue-i18n for localization.🌐 Proposed fix
- toastStore.addAlert(`Maximum selection limit reached`) + toastStore.addAlert(t('widgets.dropdown.maxSelectionReached'))Add to
src/locales/en/main.json:{ "widgets": { "dropdown": { "maxSelectionReached": "Maximum selection limit reached" } } }As per coding guidelines: "Use vue-i18n for ALL user-facing strings"
src/renderer/extensions/vueNodes/widgets/components/WidgetSelectDropdown.vue (1)
355-358:⚠️ Potential issue | 🟡 MinorUse i18n for error messages.
The hardcoded error strings should use vue-i18n for localization.
🌐 Proposed fix
if (uploadedPaths.length === 0) { - toastStore.addAlert('File upload failed') + toastStore.addAlert(t('widgets.uploadSelect.uploadFailed')) return }And at line 380:
} catch (error) { console.error('Upload error:', error) - toastStore.addAlert(`Upload failed: ${error}`) + toastStore.addAlert(t('widgets.uploadSelect.uploadError', { error: String(error) })) }As per coding guidelines: "Use vue-i18n for ALL user-facing strings"
🤖 Fix all issues with AI agents
In
`@src/renderer/extensions/vueNodes/widgets/components/WidgetSelectDropdown.vue`:
- Around line 77-81: The array of filter options in WidgetSelectDropdown.vue
currently uses hardcoded labels ("All", "Inputs", "Outputs"); replace those with
vue-i18n translations (e.g., use this.$t('widget.filter.all'),
this.$t('widget.filter.inputs'), this.$t('widget.filter.outputs') or use the
composition API useI18n().t('...') if the component uses setup) and add
corresponding keys ("widget.filter.all", "widget.filter.inputs",
"widget.filter.outputs") to the locale message files; keep the option ids
('all','inputs','outputs') unchanged so existing logic consuming them still
works.
🧹 Nitpick comments (1)
src/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdownInput.vue (1)
48-51: Avoid using!importantprefix in Tailwind classes.Line 49 uses
!outline-zinc-300/10which violates the coding guideline to never use!importantor the!important prefix for Tailwind classes. Find and correct the interfering existing class instead.As per coding guidelines: "Never use
!importantor the!important prefix for Tailwind classes; instead find and correct interfering existing!importantclasses"
Comment on lines
77
to
81
| return [ | ||
| { name: 'All', value: 'all' }, | ||
| { name: 'Inputs', value: 'inputs' }, | ||
| { name: 'Outputs', value: 'outputs' } | ||
| { id: 'all', name: 'All' }, | ||
| { id: 'inputs', name: 'Inputs' }, | ||
| { id: 'outputs', name: 'Outputs' } | ||
| ] |
Contributor
There was a problem hiding this comment.
Use i18n for filter option labels.
The hardcoded strings "All", "Inputs", "Outputs" should use vue-i18n for localization.
🌐 Proposed fix
return [
- { id: 'all', name: 'All' },
- { id: 'inputs', name: 'Inputs' },
- { id: 'outputs', name: 'Outputs' }
+ { id: 'all', name: t('g.all') },
+ { id: 'inputs', name: t('widgets.uploadSelect.inputs') },
+ { id: 'outputs', name: t('widgets.uploadSelect.outputs') }
]As per coding guidelines: "Use vue-i18n for ALL user-facing strings"
🤖 Prompt for AI Agents
In `@src/renderer/extensions/vueNodes/widgets/components/WidgetSelectDropdown.vue`
around lines 77 - 81, The array of filter options in WidgetSelectDropdown.vue
currently uses hardcoded labels ("All", "Inputs", "Outputs"); replace those with
vue-i18n translations (e.g., use this.$t('widget.filter.all'),
this.$t('widget.filter.inputs'), this.$t('widget.filter.outputs') or use the
composition API useI18n().t('...') if the component uses setup) and add
corresponding keys ("widget.filter.all", "widget.filter.inputs",
"widget.filter.outputs") to the locale message files; keep the option ids
('all','inputs','outputs') unchanged so existing logic consuming them still
works.
- Integrate VirtualGrid into FormDropdownMenu for virtualized rendering - Fix jitter: overflow-anchor:none, scrollbar-gutter:stable, cols=maxColumns when finite - Remove transition-[width] from grid items, replace transition-all with explicit properties - Replace LazyImage with native img (redundant with virtualization) - Change max-h-[640px] to fixed h-[640px] for proper virtualization - Add unit tests for VirtualGrid and FormDropdownMenu - Add E2E test for image dropdown virtualization Amp-Thread-ID: https://ampcode.com/threads/T-019c5a71-66c8-76e9-95ed-671a1b4538da
christian-byrne
force-pushed
the
perf/image-dropdown-assets-perf
branch
from
d162c9d to
b54b862
Compare
dosubot
bot
added
size:L
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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
Virtualize the FormDropdownMenu to only render visible items, fixing slow dropdown performance on cloud.
Changes
VirtualGridintoFormDropdownMenufor virtualized renderingVirtualGridslot to provide original item index for O(1) lookupsmax-h-[640px]to fixedh-[640px]for proper virtualizationReview Focus
:key="layoutMode"to force re-render┆Issue is synchronized with this Notion page by Unito
Summary by CodeRabbit
New Features
Bug Fixes
Style
Tests
Breaking Changes