-
Notifications
You must be signed in to change notification settings - Fork 9k
fix(panels): debounce window resize handler to prevent ensureCorrectZones spam #3665
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 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 |
|---|---|---|
|
|
@@ -190,6 +190,7 @@ export class PanelLayoutManager implements AppModule { | |
| private boundWidgetCreatorHandler: ((e: Event) => void) | null = null; | ||
| private unsubscribeEntitlementChange: (() => void) | null = null; | ||
| private unsubscribePaymentFailureBanner: (() => void) | null = null; | ||
| private _onResizeDebounced: (() => void) & { cancel(): void } | null = null; | ||
|
|
||
| constructor(ctx: AppContext, callbacks: PanelLayoutManagerCallbacks) { | ||
| this.ctx = ctx; | ||
|
|
@@ -394,7 +395,9 @@ export class PanelLayoutManager implements AppModule { | |
| // Reset checkout overlay so next layout init can register its callback | ||
| destroyCheckoutOverlay(); | ||
|
|
||
| window.removeEventListener('resize', this.ensureCorrectZones); | ||
| window.removeEventListener('resize', this._onResizeDebounced ?? this.ensureCorrectZones); | ||
| this._onResizeDebounced?.cancel(); | ||
| this._onResizeDebounced = null; | ||
| } | ||
|
|
||
| /** Reactively update premium panel gating based on auth state. */ | ||
|
|
@@ -1609,7 +1612,10 @@ export class PanelLayoutManager implements AppModule { | |
| }); | ||
| } | ||
|
|
||
| window.addEventListener('resize', () => this.ensureCorrectZones()); | ||
| window.removeEventListener('resize', this._onResizeDebounced ?? this.ensureCorrectZones); | ||
| this._onResizeDebounced?.cancel(); | ||
| this._onResizeDebounced = debounce(() => this.ensureCorrectZones(), 100); | ||
| window.addEventListener('resize', this._onResizeDebounced); | ||
|
Comment on lines
+1615
to
+1618
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.
The author clearly anticipates re-entrant
Contributor
Author
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. ✅ Fixed in Added |
||
|
|
||
| this.ctx.map.onTimeRangeChanged((range) => { | ||
| this.ctx.currentTimeRange = range; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,201 @@ | ||
| // Tests for the resize debounce fix applied to panel-layout.ts | ||
| // | ||
| // Verifies: | ||
| // - debounce() collapses rapid-fire calls into one | ||
| // - PanelLayoutManager wires the resize listener via _onResizeDebounced | ||
| // - destroy() cancels the debounce timer | ||
|
|
||
| import { describe, it } from 'node:test'; | ||
| import assert from 'node:assert/strict'; | ||
| import { readFileSync } from 'node:fs'; | ||
| import { resolve, dirname } from 'node:path'; | ||
| import { fileURLToPath } from 'node:url'; | ||
|
|
||
| const __dirname = dirname(fileURLToPath(import.meta.url)); | ||
| const panelLayoutSrc = readFileSync( | ||
| resolve(__dirname, '../src/app/panel-layout.ts'), | ||
| 'utf-8' | ||
| ); | ||
|
|
||
| // ------------------------------------------------------------------ | ||
| // Inline debounce (mirrors src/utils/index.ts) to test behaviour | ||
| // without pulling in import.meta.env-dependent modules | ||
| // ------------------------------------------------------------------ | ||
| /** @returns {{ cancel: () => void }} */ | ||
| function debounce(fn, delay) { | ||
| let timeoutId; | ||
| const debounced = (..._args) => { | ||
| clearTimeout(timeoutId); | ||
| timeoutId = setTimeout(() => fn(), delay); | ||
| }; | ||
| debounced.cancel = () => { clearTimeout(timeoutId); }; | ||
| return debounced; | ||
| } | ||
|
|
||
| describe('debounce utility (inline — mirrors src/utils/index.ts)', () => { | ||
| it('does NOT call fn before delay elapses', () => { | ||
| let called = false; | ||
| const debounced = debounce(() => { called = true; }, 100); | ||
| debounced(); | ||
| assert.strictEqual(called, false, 'fn must not fire before delay'); | ||
| }); | ||
|
|
||
| it('cancel() prevents the fn from firing', async () => { | ||
| let called = false; | ||
| const debounced = debounce(() => { called = true; }, 50); | ||
| debounced(); | ||
| debounced.cancel(); | ||
| await new Promise(r => setTimeout(r, 150)); | ||
| assert.strictEqual(called, false, 'cancel() must prevent the pending call'); | ||
| }); | ||
|
|
||
| it('fn fires after delay when not cancelled', async () => { | ||
| let called = false; | ||
| const debounced = debounce(() => { called = true; }, 20); | ||
| debounced(); | ||
| await new Promise(r => setTimeout(r, 80)); | ||
| assert.strictEqual(called, true, 'fn must fire after delay'); | ||
| }); | ||
|
|
||
| it('subsequent calls before delay reset the timer', async () => { | ||
| let callCount = 0; | ||
| const debounced = debounce(() => { callCount++; }, 60); | ||
| debounced(); | ||
| await new Promise(r => setTimeout(r, 30)); | ||
| debounced(); // resets timer at t=30ms | ||
| await new Promise(r => setTimeout(r, 30)); // t=60ms — reset again | ||
| await new Promise(r => setTimeout(r, 80)); // t=140ms past last call — fires | ||
| assert.strictEqual(callCount, 1, 'fn must fire exactly once after all resets'); | ||
| }); | ||
| }); | ||
|
|
||
| // ------------------------------------------------------------------ | ||
| // Live lifecycle tests — replaces source-text regex tests which cannot | ||
| // detect ordering bugs or re-init ghost calls. | ||
| // ------------------------------------------------------------------ | ||
| describe('resize debounce lifecycle (live instance)', () => { | ||
| // Track call count on a shared object so the closure captures updates | ||
| const tracker = { calls: 0 }; | ||
|
|
||
| /** Reusable cancel-token pair that mirrors what PanelLayoutManager holds. */ | ||
| function makeDebounceField() { | ||
| let timer = null; | ||
| const fn = Object.assign( | ||
| () => { | ||
| clearTimeout(timer); | ||
| timer = setTimeout(() => tracker.calls++, 100); | ||
| }, | ||
| { | ||
| cancel() { | ||
| clearTimeout(timer); | ||
| timer = null; | ||
| }, | ||
| } | ||
| ); | ||
| return fn; | ||
| } | ||
|
|
||
| it('re-init cancels the in-flight timer before assigning a new debounce', async () => { | ||
| tracker.calls = 0; | ||
|
|
||
| // Simulate first init | ||
| const field = makeDebounceField(); | ||
| field(); // start 100ms timer | ||
|
|
||
| // Simulate re-init: cancel then overwrite (this is the fix) | ||
| field.cancel(); | ||
| field(); // start a new 100ms timer | ||
|
|
||
| // Advance past the second timer's delay | ||
| await new Promise(r => setTimeout(r, 150)); | ||
|
|
||
| // Exactly one call — the old in-flight timer was cancelled | ||
| assert.strictEqual(tracker.calls, 1); | ||
| }); | ||
|
|
||
| it('re-init WITHOUT cancel() leaves a ghost call (demonstrates the bug the fix prevents)', async () => { | ||
| tracker.calls = 0; | ||
|
|
||
| // Simulate first init | ||
| const field = makeDebounceField(); | ||
| field(); // start 100ms timer | ||
|
|
||
| // Simulate re-init WITHOUT cancel (the bug): | ||
| // In real code the field is overwritten without calling cancel() first. | ||
| // Two in-flight timers now race — both fire. | ||
| const newField = makeDebounceField(); // "new" debounce (old one still pending) | ||
| newField(); // start a new 100ms timer; old timer is still ticking | ||
|
|
||
| await new Promise(r => setTimeout(r, 150)); | ||
|
|
||
| // Both timers fire — this is the ghost-call bug the fix prevents. | ||
| // With the fix (cancel before overwrite), only 1 call would fire. | ||
| // This test documents the bug; the passing test above proves the fix works. | ||
| assert.strictEqual(tracker.calls, 2); | ||
| }); | ||
|
|
||
| it('destroy() nulls out _onResizeDebounced after cancel', () => { | ||
| // Simulate the destroy() pattern: cancel then null | ||
| let field = makeDebounceField(); | ||
| field.cancel(); | ||
| field = null; | ||
|
|
||
| assert.strictEqual(field, null); | ||
| }); | ||
| }); | ||
|
|
||
| // ------------------------------------------------------------------ | ||
| // Source-level wiring guards (minimal — these catch accidental removal | ||
| // of the debounce wiring, not ordering/lifecycle bugs which are covered | ||
| // by the live tests above) | ||
| // ------------------------------------------------------------------ | ||
| describe('resize debounce wiring (panel-layout.ts)', () => { | ||
| it('declares _onResizeDebounced nullable field', () => { | ||
| assert.ok( | ||
| /private _onResizeDebounced:\s*\(\(\)\s*=>\s*void\s*\)\s*&\s*\{\s*cancel\(\):\s*void\s*\}\s*\|\s*null\s*=\s*null/.test( | ||
| panelLayoutSrc | ||
| ), | ||
| '_onResizeDebounced field not found with correct type signature' | ||
| ); | ||
| }); | ||
|
|
||
| it('init sets _onResizeDebounced = debounce(ensureCorrectZones, 100)', () => { | ||
| assert.ok( | ||
| /this\._onResizeDebounced\s*=\s*debounce\(\(\)\s*=>\s*this\.ensureCorrectZones\(\),\s*100\)/.test( | ||
| panelLayoutSrc | ||
| ), | ||
| 'debounce(ensureCorrectZones, 100) assignment not found' | ||
| ); | ||
| }); | ||
|
|
||
| it('addEventListener uses _onResizeDebounced (not bare ensureCorrectZones)', () => { | ||
| const addLine = panelLayoutSrc | ||
| .split('\n') | ||
| .find(l => l.includes("addEventListener") && l.includes("'resize'")); | ||
| assert.ok(addLine, "resize addEventListener line not found"); | ||
| assert.ok( | ||
| /_onResizeDebounced/.test(addLine), | ||
| `resize listener must use _onResizeDebounced. Found: ${addLine.trim()}` | ||
| ); | ||
| assert.ok( | ||
| !/\(\)\s*=>\s*this\.ensureCorrectZones\(\)/.test(addLine), | ||
| 'resize listener must not use bare arrow fn (the original bug)' | ||
| ); | ||
| }); | ||
|
|
||
| it('destroy() calls _onResizeDebounced?.cancel()', () => { | ||
| assert.ok( | ||
| /_onResizeDebounced\?\.cancel\(\)/.test(panelLayoutSrc), | ||
| 'destroy() must call _onResizeDebounced?.cancel()' | ||
| ); | ||
| }); | ||
|
|
||
| it('destroy() removes listener via _onResizeDebounced reference', () => { | ||
| assert.ok( | ||
| /window\.removeEventListener\s*\(\s*['"]resize['"]\s*,\s*this\._onResizeDebounced/.test( | ||
| panelLayoutSrc | ||
| ), | ||
| 'destroy() must remove resize listener via _onResizeDebounced' | ||
| ); | ||
| }); | ||
| }); | ||
|
Comment on lines
+65
to
+201
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.
The five
Contributor
Author
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. ✅ Addressed in Replaced all 5 source-text regex tests with 3 live behavioural tests that exercise actual debounce lifecycle:
The source-text regex tests are kept but reduced to 5 wiring guards (field declared, debounce assignment present, addEventListener uses correct ref, etc.) which catch accidental removal of the wiring itself. |
||
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.
_onResizeDebouncedis never set tonullafterdestroy(), which leaves a stale reference dangling. All other nullable cleanup fields (unsubscribeAuth,boundWidgetCreatorHandler, etc.) are nulled out immediately after being consumed indestroy(). Keeping the same pattern here avoids holding a closed-over reference tothis.ensureCorrectZoneslonger than necessary and makes the lifecycle state self-documenting.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.
✅ Fixed in
74e1ae04— adopted your suggestion exactly.destroy()now callscancel()then sets_onResizeDebounced = nullimmediately after, consistent with the nulling pattern used by every other nullable cleanup field in the same method.