perf/throttle resize handler#476
Merged
Merged
Conversation
gnbm
commented
Jun 8, 2026
gnbm
commented
Collaborator
- perf: throttle global resize handler and guard against instance-less wrappers (P1)
- Update utils.js
- Add typings to throttle and use spread
- fix(utils): preserve caller context in throttle (Copilot review)
- fix: prevent Virtual Select memory leaks (document click listener + cleanup gaps) (fix: prevent Virtual Select memory leaks (document click listener + cleanup gaps) #468)
- build: rebuild minified bundles with S2/S3/P1 fixes
- refactor: keep stable reference to throttled resize handler (Copilot review)
- Potential fix for pull request finding
- Revert "Potential fix for pull request finding"
- refactor: drop unnecessary static class field for onResizeThrottled (Copilot review)
- fix(utils): release throttle references after invocation (Copilot review)
- chore: clear pending timers on destroy + clarify reflow/highlight intent (chore: clear pending timers on destroy + clarify reflow/highlight intent #473)
…wrappers (P1) The window 'resize' listener ran onResizeMethod on every tick, which queried all .vscomp-ele-wrapper elements and recomputed each instance's options-container height - O(instances) layout work per resize event during a drag. It also assumed every wrapper's parent had a .virtualSelect and threw otherwise. - Add Utils.throttle and wrap the resize listener (~100ms) so the per-instance work runs at most ~10x/sec. - Guard onResizeMethod against wrappers whose instance is missing (teardown races). Closed instances are still updated (opening does not recompute height, so skipping them could leave a stale height on reopen) - throttling delivers the win without that regression risk. Adds cypress spec perf-resize-throttle.cy.ts (throttling, no-regression, guard).
Fix lint issues
Add JSDoc/type annotations to Utils.throttle (timeout and lastArgs) and annotate the throttled function parameter. Replace callback.apply(this, lastArgs) with callback(...lastArgs) for simpler invocation and improved type-safety. Updated the built/minified distribution and docs asset files accordingly.
Invoke the throttled callback via callback.apply(lastThis, lastArgs) on both leading and trailing edges so instance methods used as throttled event handlers run with the expected 'this'. Add @this annotation to satisfy tsc. Regenerate dist/docs bundles.
…leanup gaps) (#468) * fix: remove document click listener correctly to prevent memory leak onDocumentClick is added to `document` in the capture phase (addEvent(document, 'click', 'onDocumentClick', true)), but removeEvent called removeEventListener without the capture flag. removeEventListener only unregisters a listener when its capture flag matches the one used at registration, so the capture-phase document listener was never removed. As a result, every render/destroy cycle (e.g. repeatedly opening and closing the dropdown) left a new onDocumentClick listener bound to `document`. Each leaked listener retains the VirtualSelect instance and its detached dropbox DOM subtree, so event-listener and DOM-node counts grow without bound and are never reclaimed by GC. Fix: thread the capture flag through DomUtils.removeEvent and the VirtualSelect.removeEvent wrapper, and pass capture: true when removing the document click listener so add and remove match. * fix: ensure cleanup runs in inline mode and clear retained references on destroy Two additional memory-leak hardening fixes: 1. Install the self-destroy MutationObserver in every mode, not only when hasDropboxWrapper is true. Previously, inline-mode dropdowns had no observer, so removing the host element from the DOM without calling destroy() left all addEvents() listeners attached - including the capture-phase document click listener that retains the instance and its detached DOM. removeMutationObserver now disconnects based on the observer's existence. 2. On destroy(), clear the $dropboxWrapper.virtualSelect back-reference (set in setEleProps) before detaching the wrapper, and reset this.events. This breaks the instance <-> detached-DOM reference cycle so the instance and its DOM can be reclaimed. * fix: limit mutation observer to popup mode to avoid per-instance perf penalty Reverts the all-modes MutationObserver install. In the default dropboxWrapper:'self' mode every instance was registering a body-wide subtree observer, adding O(instances x mutations) overhead on pages with many selects. The reported document-click listener leak is fixed by the capture-flag correction in removeEvents(); the observer stays scoped to popup mode as before.
The branch committed the unminified dist/docs sources and the dist-archive copy with the security (S2 customData escaping, S3 classNames sanitization) and perf (P1 throttled resize) fixes, but the primary distributed bundles (dist/virtual-select.min.js and docs/assets/virtual-select.min.js) were left stale - consumers loading them got none of the fixes. Regenerated via 'npm run build' so all three min bundles share an identical body and match the source.
…review) The throttled wrapper passed inline to addEventListener was anonymous, so the resize listener could no longer be detached via removeEventListener (the prior VirtualSelect.onResizeMethod reference was removable). Store the throttled function on VirtualSelect.onResizeThrottled and register that reference instead, restoring removability for future cleanup. Declared as a typed static field so it passes tsc (checkJs + strict).
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This reverts commit 98c9649.
…Copilot review) The static class-field declaration provided no type-safety benefit (the file is // @ts-nocheck, so tsc never checks it) and used ES2022 class-field syntax not present anywhere else in the codebase. The property is assigned at module init time via VirtualSelect.onResizeThrottled = ..., which is sufficient to expose a stable, removable reference. Runtime behaviour is unchanged.
…iew) throttle retained lastArgs/lastThis (e.g. a DOM Event passed to a resize handler) until the next call or indefinitely if never called again. Clear them after both the leading and trailing invocations so large arguments aren't held, matching the canonical underscore/lodash throttle. Behaviour is unchanged: both paths reassign lastArgs/lastThis at the top of every call before use.
…ent (#473) * chore: clear pending timers on destroy + clarify reflow/highlight intent (S4/P3/P4) P4: route fire-and-forget setTimeout calls through setManagedTimeout, whose ids are tracked and cleared in destroy(). Prevents callbacks (e.g. renderOptions, focus) from running against a destroyed instance. P3: document the intentional forced reflow in openDropbox so it is not removed as a no-op. S4: document that the search-highlight regex relies on enableSecureText for escaping and is not an additional injection vector (input is already regex-escaped). Adds cypress spec timer-cleanup.cy.ts. * Improve throttle typings and clean test comment Add JSDoc types to Utils.throttle (typed timeout and lastArgs, and typed throttled args) and switch callback.apply to a spread invocation to simplify typing. Also initialize lastArgs as an array instead of null. In the test, remove an extraneous @ts-expect-error comment. These tweaks improve TypeScript/JSDoc compatibility and tidy up the test file. * Refactor Utils.throttle to use spread args Add JSDoc types for timeout and lastArgs and initialize lastArgs as an array. Replace callback.apply(this, lastArgs) with callback(...lastArgs) in Utils.throttle to simplify invocation and improve type-awareness. Updated built artifacts and docs assets (dist and docs/assets) accordingly. * Update virtual-select-1.2.0.min.js * Preserve context in throttle and clarify comment Capture and preserve the calling context in Utils.throttle by converting the returned function to a named function, adding JSDoc for `this`/args, storing `this` to `context`, and invoking the callback with `callback.apply(context, lastArgs)`. Return the throttled function explicitly. Also reword a comment in virtual-select to clarify that turning off enableSecureText is an explicit consumer choice to render raw HTML, so the added highlight does not introduce an extra injection vector. * fix(utils): remove broken throttle edit; harden timer-cleanup test - Remove unused 'const context = this' and unreachable out-of-scope 'return throttled;' in Utils.throttle (Copilot review). Source had drifted from the already-correct dist bundle; restores parity, no behavior change. - Use cy.clock()/cy.tick() in timer-cleanup test so the negative 'timer must not fire after destroy' assertion is deterministic and not masked by CI timer throttling (Copilot review). * build: sync bundles with committed source (stale highlight comment) Regenerated dist/docs bundles were out of sync with an already-committed source comment rewording. No code/behavior change; throttle output is unchanged. Separated from the fix commit since it is pre-existing drift.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to improve runtime performance and robustness of Virtual Select by throttling the global resize work and preventing timer callbacks from running after an instance is destroyed.
Changes:
- Throttle the global
window.resizehandler and guard against wrapper elements without an attached instance. - Introduce
setManagedTimeout()/clearManagedTimeouts()and migrate severalsetTimeoutusages to prevent post-destroy callbacks. - Add Cypress coverage for resize throttling and managed-timeout cleanup.
Reviewed changes
Copilot reviewed 5 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/virtual-select.js | Uses managed timeouts, clears them on destroy(), and throttles/guards the global resize handler. |
| src/utils/utils.js | Adds Utils.throttle() used for resize throttling. |
| docs/assets/virtual-select.js | Rebuilt docs bundle reflecting the changes (but should not be committed per repo PR guidance). |
| dist/virtual-select.js | Rebuilt distribution bundle reflecting the changes (but should not be committed per repo PR guidance). |
| dist/virtual-select.min.js | Rebuilt minified distribution bundle (but should not be committed per repo PR guidance). |
| cypress/e2e/timer-cleanup.cy.ts | New e2e tests verifying managed timeouts don’t fire after destroy(). |
| cypress/e2e/perf-resize-throttle.cy.ts | New e2e tests verifying resize throttling and guarding against instance-less wrappers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+258
to
+277
| if (remaining <= 0 || remaining > wait) { | ||
| if (timeout) { | ||
| clearTimeout(timeout); | ||
| timeout = null; | ||
| } | ||
| previous = now; | ||
| callback.apply(lastThis, lastArgs); | ||
| /** release references so a large last argument (e.g. a DOM Event) isn't retained */ | ||
| lastArgs = []; | ||
| lastThis = undefined; | ||
| } else if (!timeout) { | ||
| timeout = setTimeout(() => { | ||
| previous = Date.now(); | ||
| timeout = null; | ||
| callback.apply(lastThis, lastArgs); | ||
| /** release references so a large last argument (e.g. a DOM Event) isn't retained */ | ||
| lastArgs = []; | ||
| lastThis = undefined; | ||
| }, remaining); | ||
| } |
gnbm
added a commit
that referenced
this pull request
Jun 9, 2026
…ancy hardening (#478) * perf: ref-count global listeners/observer + add throttle cancel Follow-up to the memory-leak / performance series (#468, #475, #476, #477). Three lifecycle items called out as optional follow-ups during review of that series. No API change and no behaviour change while any instance is alive. - Disconnect the shared MutationObserver when no instances remain. disconnectDomObserver() disconnects and nulls VirtualSelect.domObserver when the last live instance is destroyed (re-created lazily if a new instance appears). - Add Utils.throttle().cancel(). The throttled wrapper kept a trailing setTimeout that could fire after its listener was removed. cancel() clears the pending timer and releases the retained this/args. Return type is now ThrottledFunction (callable + cancel()). - Lazy, ref-counted page-level listeners. resize/reset/submit are now attached on the first instance (registerInstance -> addGlobalListeners) and removed when the last instance is destroyed (unregisterInstance -> removeGlobalListeners), using stable handler references so removeEventListener actually unregisters them. removeGlobalListeners also cancels onResizeThrottled and triggers disconnectDomObserver(). Verified: tsc --strict and eslint pass; dist/, docs/assets/ and dist-archive/ rebuilt with the new symbols. * test: add observer/listener lifecycle spec; fix stale resize-handler comment - Add cypress/e2e/observer-listener-lifecycle.cy.ts covering the three follow-up behaviours: the shared MutationObserver and page-level resize/reset/submit listeners are torn down when the last instance is destroyed (and the resize listener is removed via the same stable reference), both are re-established lazily when a new instance appears, the shared observer is not duplicated for additional instances, and Utils.throttle().cancel() (via VirtualSelect.onResizeThrottled) prevents a queued trailing call from firing (with a positive control showing the trailing call fires when not cancelled). - Update the stale "near the global resize listener" comment in src/virtual-select.js (and the regenerated dist/docs bundles) to reflect that the page-level listeners are now attached lazily in addGlobalListeners() on the first instance, not at module scope. * fix: add missing Utils.sanitizeClassNames helper src/virtual-select.js calls Utils.sanitizeClassNames(d.classNames) when an option supplies classNames, but the helper was never added to the utils module (it lives only on the unmerged security/sanitize-classnames branch). As shipped, any option with classNames throws "Utils.sanitizeClassNames is not a function" at render time, breaking option rendering and leaving the intended attribute-injection hardening absent. Add the helper: strip the characters that could break out of the double-quoted class attribute (" < >). Valid CSS class tokens never contain these, so legitimate class names are untouched. null/undefined pass through. Note: dist/docs bundles must be regenerated (npm run build) before release. * perf: harden Utils.throttle against re-entrancy The throttled wrapper cleared lastArgs/lastThis AFTER invoking the callback. If the callback re-enters (calls the throttled function again, directly or indirectly), the post-call cleanup would wipe the re-entrant invocation's freshly captured args/this, so a queued trailing call could later run with the wrong (empty) context/args. Route both the leading- and trailing-edge calls through a small invoke() helper that snapshots lastThis/lastArgs and clears them BEFORE calling, so a re-entrant call captures its own state. Behaviour is unchanged for the current sole consumer (the window-resize handler, which does not re-enter); this is defensive hardening of the shared utility. Clearing before the call also preserves the existing "don't retain a large last argument" guarantee. Note: dist/docs bundles must be regenerated (npm run build) before release. * chore: rebuild bundles for sanitizeClassNames + throttle hardening * test: give throttle specs a non-zero fake-clock base The two throttle specs used cy.clock(), which starts the fake clock at the epoch (now = 0). Combined with cancel() resetting previous = 0, the leading edge condition (now - previous > wait) was never satisfied, so the first onResizeThrottled() call scheduled a trailing call instead of firing immediately. Both specs then asserted a leading invocation that never happened and failed. Use cy.clock(1_000_000) so now - previous exceeds wait and the leading edge fires synchronously, matching production where Date.now() is always a large timestamp. No product change; the throttle behaviour was correct.
This was referenced Jun 10, 2026
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
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.