-
Notifications
You must be signed in to change notification settings - Fork 9k
fix(panels): debounce window resize handler to prevent ensureCorrectZones spam #4000
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
a0ed038
a9d8891
f9d6ed4
80b2ff1
7994431
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,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 | ||
|
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 test titled "re-init cancels the in-flight timer before assigning a new debounce" calls Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
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. Thanks — addressed in f9d6ed4: f9d6ed4 Rewrote the test to use two separate
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. Thanks — addressed in f9d6ed4. Rewrote the 're-init cancels in-flight timer' test to use two separate makeDebounceField() instances (cancel first, then create new closure) — mirrors the production fix where _onResizeDebounced = debounce(...) assigns an entirely new closure. Now actually exercises the two-closure replacement path, not just cancel() reuse. All 12 tests pass. Commit: f9d6ed4 |
||
| 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' | ||
| ); | ||
| }); | ||
| }); | ||
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.
removeEventListenercallsBoth the
destroy()andinit()re-entry paths fall back tothis.ensureCorrectZoneswhen_onResizeDebouncedisnull, but the barethis.ensureCorrectZonesmethod reference was never registered as the resize listener — not in the old code (which used() => this.ensureCorrectZones(), an anonymous arrow fn), and not in the new code (which always uses_onResizeDebounced). The fallback can therefore never match a registered listener, making bothremoveEventListener(…, … ?? this.ensureCorrectZones)calls silent no-ops whenever_onResizeDebouncedisnull. The same pattern appears at line 1629 ininit(). Consider dropping the?? this.ensureCorrectZonesarm to avoid misleading future readers into thinking the bare method reference is ever meaningful here.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.
Thanks — addressed in f9d6ed4: f9d6ed4
Dropped the
?? this.ensureCorrectZonesfallback from both removeEventListener call sites. The bare method reference was never registered as a listener (old code used an anonymous arrow fn, new code uses_onResizeDebounced), so the fallback was always dead code. Both sites now passthis._onResizeDebounceddirectly, which is correct whether null (silent no-op) or not.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.
Thanks — addressed in f9d6ed4. Dropped ?? this.ensureCorrectZones from both
emoveEventListener calls in destroy() and init(). The bare method reference was never registered as a listener (old code used an anonymous arrow fn, new code uses _onResizeDebounced), so the fallback was indeed dead code. Both spots in panel-layout.ts now pass _onResizeDebounced directly (relying on ?. chaining for the null case). All 12 tests pass. Commit: f9d6ed4